Bug/1525 using code instead of name broke existing projects #1531
Conversation
WalkthroughThis pull request refactors project-related handling across both backend and frontend code. Method signatures and parameter names have been updated to replace ambiguous "name" parameters with clearer identifiers like "code" or "projectId". Changes include adding extra parameters in project creation, updating record and constructor definitions for project metadata, revising logging and error handling, and modifying routing and component properties in the Svelte frontend. Migration files have been added to update the database schema with a new required column, and XML documentation improvements have been made. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant View as DotnetProjectView
participant PS as ProjectServicesProvider
Note over View,PS: Opening a project using projectId
User->>View: Navigate to project page with projectId
View->>PS: getCrdtProjectData(projectId)
PS-->>View: Return project data (with name and code)
View->>PS: openCrdtProject(projectId)
PS-->>View: Return project scope
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
UI unit Tests12 tests 12 ✅ 0s ⏱️ Results for commit 1e207a3. ♻️ This comment has been updated with latest results. |
C# Unit Tests107 tests 107 ✅ 5s ⏱️ Results for commit 1e207a3. ♻️ This comment has been updated with latest results. |
![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 (4)
backend/FwLite/MiniLcm/Models/ProjectIdentifier.cs (1)
5-7: Consider clarifying the documentation about Name uniqueness.The documentation addition is valuable, but describing Name as a "unique identifier" might be misleading if the PR is moving away from using names as primary identifiers toward using codes/IDs instead.
/// <summary> -/// A human-readable unique identifier for the project +/// A human-readable name for the project, which may not be unique across all systems /// </summary>backend/FwLite/LcmCrdt/CurrentProjectService.cs (1)
20-20: Consider maintaining explicit type annotation for clarityWhile the compiler can infer the type, removing
ProjectData?makes the nullability less explicit, which is important given the null check on line 21. Explicit type annotations improve code readability and make nullability expectations clearer.-var result = LookupProjectData(memoryCache, Project); +ProjectData? result = LookupProjectData(memoryCache, Project);backend/FwLite/LcmCrdt/CrdtProjectsService.cs (2)
89-89: Consider differentiating example code from name.For the example project, you're using the same string for both name and code. While this works, it might be clearer to use a distinct code to demonstrate the separation of concerns.
96-101: Updated validation to focus on code rather than name.Properly updated validation method to reference code instead of name. Consider enhancing the error message to include the expected format based on the regex.
- var nameIsInvalid = $"Project code '{request.Code}' is invalid"; + var nameIsInvalid = $"Project code '{request.Code}' is invalid. Expected format: alphanumeric with hyphens and underscores, must start with a letter or number.";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
backend/FwHeadless/Services/SyncHostedService.cs(1 hunks)backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs(1 hunks)backend/FwLite/FwLiteProjectSync/Program.cs(2 hunks)backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs(7 hunks)backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs(1 hunks)backend/FwLite/FwLiteShared/Sync/BackgroundSyncService.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs(1 hunks)backend/FwLite/LcmCrdt/CrdtProject.cs(2 hunks)backend/FwLite/LcmCrdt/CrdtProjectsService.cs(5 hunks)backend/FwLite/LcmCrdt/CurrentProjectService.cs(1 hunks)backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.Designer.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs(1 hunks)backend/FwLite/LcmCrdt/RemoteSync/CrdtHttpSyncService.cs(1 hunks)backend/FwLite/MiniLcm/Models/ProjectIdentifier.cs(1 hunks)frontend/viewer/src/App.svelte(1 hunks)frontend/viewer/src/DotnetProjectView.svelte(1 hunks)frontend/viewer/src/home/HomeView.svelte(3 hunks)frontend/viewer/src/home/ProjectTitle.svelte(1 hunks)frontend/viewer/src/home/Server.svelte(5 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/ICombinedProjectsService.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/IProjectModel.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectServicesProvider.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectData.ts(1 hunks)frontend/viewer/src/lib/services/projects-service.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: check-and-lint
- GitHub Check: Analyze (csharp)
- GitHub Check: Build FW Lite and run tests
🔇 Additional comments (65)
frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectData.ts (1)
9-9: Property addition aligns with PR objectives.Adding the
codeproperty toIProjectDatasupports the PR goal of saving both project code and name within the ProjectData structure, enabling the code to serve as a reliable filename while still allowing the display of the human-readable name.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/IProjectModel.ts (1)
11-11: Addition of code property enhances project identification.Adding the
codeproperty toIProjectModelsupports the PR objective of utilizing more reliable identifiers than names alone. This will help ensure projects can be consistently located regardless of naming conventions used for SQLite file names.backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs (1)
1-1: Cleanup of unused imports improves code cleanliness.Removing unnecessary using directives (
System.Data.Common,Microsoft.Data.Sqlite,Microsoft.EntityFrameworkCore.Diagnostics) reduces cognitive load and makes the file more maintainable.Let's verify that these imports are indeed no longer needed:
#!/bin/bash # Check if any of the removed imports are still needed somewhere in the file rg -A 5 -B 5 "SqliteConnection|DbConnection|InterceptResult" backend/FwLite/LcmCrdt/LcmCrdtDbContext.csfrontend/viewer/src/home/ProjectTitle.svelte (1)
1-13: Well-designed component for displaying project titlesThis new component elegantly handles the display of project identifiers by showing both the name and code (when different) with appropriate styling. The conditional logic on line 7 ensures that redundant information isn't displayed when the code matches the name.
backend/FwLite/LcmCrdt/RemoteSync/CrdtHttpSyncService.cs (1)
70-70: Updated constructor parameters match the new ProjectData signatureThe
TestAuthmethod has been updated to provide a code parameter (set to "test") for theProjectDataconstructor, aligning with the changes to theProjectDataclass structure that now requires both name and code parameters.backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs (1)
25-28: Project creation updated to include code parameterThe
Importmethod now correctly passes both name and code parameters to the project creation call. Using the same value for both parameters is appropriate in this context, as this handles the migration of existing FwData projects where code and name would initially be the same.backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs (1)
30-33: Database schema properly updated for new Code fieldThe migration adds the required
Codecolumn to theProjectDatatable with the appropriate type settings. This change supports the new approach of using project codes for identification while preserving names for display.backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.cs (1)
1-37: LGTM! Well-designed migration for adding the Code columnThe migration follows best practices by:
- First adding the column as nullable
- Populating it with values from the Name column
- Then altering it to be non-nullable
This approach ensures a smooth migration for existing data without constraint violations.
backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs (1)
52-52: Correctly updated test to match new ProjectData constructor signatureThe constructor call now includes the additional "sena-3" code parameter, properly aligning with the changes made to the ProjectData record structure.
backend/FwLite/LcmCrdt/CurrentProjectService.cs (1)
24-24: LGTM! Good simplificationDirectly using
CacheKey(Project)without storing it in a temporary variable is cleaner.backend/FwHeadless/Services/SyncHostedService.cs (1)
200-202:❓ Verification inconclusive
Verify the first parameter's purpose
You've added "crdt" as a second parameter to CreateProject, but it's identical to the first parameter. Ensure these parameters serve different purposes (e.g., name vs code) to avoid confusion.
🏁 Script executed:
#!/bin/bash # Look for the CreateProject method signature to understand the parameters rg -A 10 "CreateProject.*\(" --type csharp backend/Length of output: 36345
Clarify the Project Name vs. Code in CreateProject
After reviewing the related code, it’s clear that the two string parameters in the CreateProject request represent the project’s name and code, respectively. We see that in other parts of the codebase (e.g., in FwLiteProjectSync and CrdtProjectsService) the same value is sometimes passed for both parameters when the project name and code are intended to be identical.
• In backend/FwHeadless/Services/SyncHostedService.cs (lines 200–202), both parameters are set to
"crdt".
• The CreateProjectRequest record in CrdtProjectsService.cs defines the first parameter as the name and the second as the code.Please verify that passing
"crdt"for both the name and code is intentional. If they’re meant to be different, update the second parameter accordingly. Otherwise, consider adding a clarifying inline comment to highlight that using the same string in both positions is by design.frontend/viewer/src/lib/services/projects-service.ts (1)
18-18: Method signature improvementThe method signature for
downloadProjecthas been updated to accept a project model object instead of individual string parameters. This is a good change that simplifies the API and aligns with the broader refactoring effort to use consistent project identifiers.backend/FwLite/FwLiteProjectSync/Program.cs (3)
49-49: Variable name updated for consistencyRenaming
crdtProjectNametocrdtProjectCodereflects the intended purpose of using the file name as a code rather than a display name, which aligns well with the PR objectives.
63-63: Method call updated to use project codeUpdated to use
projectsService.GetProject(crdtProjectCode)with the new variable name, maintaining consistency in how projects are retrieved.
66-66: Project creation now uses consistent identifiersThe project creation now explicitly uses
crdtProjectCodefor both the code and name parameters, ensuring consistency between identifiers. This addresses the core issue in bug #1525 where projects couldn't be located due to inconsistent naming.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectServicesProvider.ts (2)
8-8: Added import for project data interfaceThis new import supports the additional functionality for retrieving project data.
14-15: API enhancement to work with project IDsTwo important interface changes:
- Added new method
getCrdtProjectDatato retrieve project data using project ID- Updated
openCrdtProjectto useprojectIdinstead ofprojectNameThese changes improve the interface by consistently using project IDs throughout, making project identification more reliable.
frontend/viewer/src/home/HomeView.svelte (5)
26-26: Added ProjectTitle component importGood addition that will standardize how project titles are displayed across the application.
62-66: Function updated to use project IDThe
deleteProjectfunction now accepts and usesprojectIdinstead ofprojectName, aligning with the broader changes to use consistent project identifiers.
152-153: Updated routing to use project IDsThe link now uses
project.idinstead ofproject.namefor routing, which provides more reliable navigation since IDs are guaranteed to be unique and consistent.
155-156: Improved project title displayReplaced hardcoded title with the new
ProjectTitlecomponent, which likely handles the logic for displaying either the project name, code, or both based on context.
161-163: Added error checking for project IDGood defensive programming - the code now checks that
project.idexists before attempting to use it. This prevents potential runtime errors and provides a clear error message.frontend/viewer/src/App.svelte (1)
76-79: Route parameter changed from 'name' to 'id' for CRDT projectsThe route now uses project ID instead of project name, which aligns with the PR objective of using project IDs for consistent project identification.
frontend/viewer/src/home/Server.svelte (4)
10-10: Added ProjectTitle component importGood addition to separate the display logic for project titles.
29-29: Simplified project download APINow passing the entire project object instead of separate ID and name properties. This simplification makes the API more consistent and future-proof.
87-91: Project identification and display updatesChanged from using project.name to project.id in the href attribute and replaced the direct title display with the ProjectTitle component. These changes align with the PR objective of using project IDs for routing while maintaining a user-friendly display.
102-103: Consistent project title displayUsing the ProjectTitle component here maintains consistency with the earlier instance. Good practice.
backend/FwLite/FwLiteShared/Sync/BackgroundSyncService.cs (3)
25-26: Updated project retrieval methodChanged from using CurrentProjectService.LookupProjectById to crdtProjectsService.GetProject for consistency with other project access patterns.
32-36: Improved null check and error messageBetter error handling by explicitly checking for null project data and providing a clear warning message.
38-42: Updated client ID checkReorganized the client ID check to work with the new data structure, maintaining the same functionality while adapting to the updated project data access pattern.
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (4)
41-46: Added GetCrdtProjectData methodGood addition that provides direct access to project data using a GUID. The error handling is thorough, throwing appropriate exceptions when the project or its data is not found.
49-50: Updated OpenCrdtProject method signatureChanged from using string to Guid for project identification, which aligns with the PR objective of using project IDs instead of names.
53-55: Updated project retrievalNow using the more direct crdtProjectsService.GetProject method with proper error handling. This change is consistent with the overall approach to use GUIDs for project identification.
62-64: Updated logging to use project name from dataThe log message now correctly references the project name from the project data, which provides more useful information in the logs.
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/ICombinedProjectsService.ts (2)
16-16: Parameter signature refactored for improved clarity and consistency.The method now accepts a unified
projectobject of typeIProjectModelinstead of separate parameters, which aligns with the PR objective of handling projects by their IDs rather than names.
18-18: Parameter renamed from 'name' to 'projectId' for better semantic clarity.This change makes it explicit that the method requires a project identifier rather than a display name, improving code readability and preventing potential confusion.
frontend/viewer/src/DotnetProjectView.svelte (5)
14-19: Props restructured to use projectId instead of projectName.The component now accepts
projectIdandtypeas props, with a helpful type comment clarifying that projectId can be a GUID for CRDTs or a project name for FWData. This distinction matches the PR objective of improving project identification.
21-21: Added state variable for projectName.This change allows the component to manage the display name separately from the identifier, supporting the PR objective of separating name (for display) from code/ID (for routing).
23-23: Convert serviceLoaded to a state variable.Using
$state()instead of a simple boolean makes this reactive, which is a good practice in Svelte.
27-30: Updated CRDT project loading logic.The component now retrieves the project data first to get the display name, then opens the project using its ID. This implements the PR's core objective of using project IDs for routing while preserving display names.
32-33: Updated fwdata project loading logic.For fwdata projects, the name is set to match the ID, maintaining backward compatibility while aligning with the new structure.
backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs (7)
11-14: Added Code property to ProjectModel record.This change separates the project's display name from its unique code used for file naming and routing, addressing the core issue described in the PR.
20-26: Updated ApiEndpoint property to use Code instead of Name.This ensures that API endpoints are constructed using the reliable project code rather than potentially changing display names.
57-65: Updated ProjectModel instantiation with both Name and Code.Now correctly maps Lexbox project properties to the local model, maintaining the distinction between the display name and the internal code.
83-90: Enhanced project mapping in LocalProjects method.The comment on line 83 confirms that
p.Nameactually contains the project code, which is now properly assigned. The new approach also correctly sets the display name fromp.Data!.Name.
102-103: Updated ProjectModel instantiation for non-CRDT projects.For projects that only exist in FWData format, the code now sets both Name and Code to the same value, which is reasonable when no separate code is available.
112-126: Refactored DownloadProject method.Now accepts a complete ProjectModel object instead of separate parameters, and correctly uses the project's Name, Code, and Id properties when creating the project. The null check for project.Id ensures data integrity.
135-138: Updated DeleteProject method to use projectId.Changed from string name to Guid projectId, which provides a more reliable way to identify projects, especially when names might be duplicated.
backend/FwLite/LcmCrdt/CrdtProject.cs (5)
5-6: Constructor parameter renamed from 'name' to 'code'.This change reflects the true nature of this parameter, which is the project code rather than display name, addressing the core issue where using code instead of name broke existing projects.
7-10: Updated overloaded constructor to use 'code' parameter.Consistently applies the parameter renaming to all constructors.
12-16: Added clarifying documentation for the Name property.This XML comment is crucial as it explains that despite being called "Name", this property actually contains the Lexbox project code. This helps prevent future confusion.
24-26: Updated parameter documentation for clarity.The documentation now properly distinguishes between Name (display name) and Code (unique identifier), which aligns with the changes throughout the codebase.
30-30: Updated ProjectData record to include Code parameter.This is the fundamental change that enables the separation between display name and identification code, addressing the core issue mentioned in the PR.
backend/FwLite/LcmCrdt/CrdtProjectsService.cs (9)
22-24: Parameter renamed for clarity.Parameter renamed from
nametocodeto better represent its purpose as a project identifier rather than a display name. This change aligns with the PR objective to use project IDs instead of names for project identification.
58-63: Method updated to use code-based project identification.Updated method to find projects using code instead of name, aligning with the new identification approach.
65-68: Useful addition for GUID-based project lookup.Adding this overload is a good practice that will make project identification more flexible. The implementation correctly uses the project ID from the data.
70-73: Method updated to use code-based identification.Good update to ProjectExists to maintain consistent terminology throughout the class.
75-85: Record updated to separate project code from name.Adding the Code property to CreateProjectRequest allows for separate handling of the display name and the file identifier, which addresses the core issue in ticket #1525.
104-105: Updated file path construction to use code.This change ensures SQLite files are named after the project code rather than display name, addressing the issue mentioned in the PR objectives.
120-122: Updated ProjectData construction to include both name and code.Good implementation that preserves both the display name and the code identifier in the model.
171-174: Method signature updated to use GUID instead of string.Using GUID for project identification provides better uniqueness and type safety compared to string names. The implementation correctly retrieves the project by ID.
196-196: Renamed regex method to reflect its purpose.Method name updated from ProjectName to ProjectCode to better reflect what it's validating.
backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.Designer.cs (3)
15-16: Appropriate migration name.The migration name "AddCodeToProjectData" clearly describes the purpose of this database change.
24-56: ProjectData entity properly configured with new Code field.The ProjectData entity has been updated to include the required "Code" property as a TEXT column. This is properly configured with IsRequired() to ensure it's a non-nullable field.
1-639: Migration file is properly auto-generated.This is an auto-generated EF Core migration file that correctly defines the database schema with the new Code field and all related entities. No manual edits needed as this follows standard EF Core migration patterns.
myieye
force-pushed
the
bug/1525-using-code-instead-of-name-broke-existing-projects
branch
2 times, most recently
from
f21b35c to
cf66405
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (7)
backend/FwLite/FwLiteWeb/Routes/ProjectRoutes.cs (3)
31-37: Parameter name could be updated for clarity.While the validation now uses
ProjectCode(), the parameter is still namedname, which could be confusing. Consider renaming tocodeorprojectCodeto better reflect its purpose and maintain consistency with the PR objectives.- async (CrdtProjectsService projectService, string name) => + async (CrdtProjectsService projectService, string projectCode) => { - if (string.IsNullOrWhiteSpace(name)) - return Results.BadRequest("Project name is required"); - if (projectService.ProjectExists(name)) + if (string.IsNullOrWhiteSpace(projectCode)) + return Results.BadRequest("Project code is required"); + if (projectService.ProjectExists(projectCode)) return Results.BadRequest("Project already exists"); - if (!CrdtProjectsService.ProjectCode().IsMatch(name)) + if (!CrdtProjectsService.ProjectCode().IsMatch(projectCode)) return Results.BadRequest("Only letters, numbers, '-' and '_' are allowed");
55-55: Parameter name could be updated for clarity.Since the validation now uses
ProjectCode(), consider renaming theprojectNameparameter toprojectCodeto better reflect its purpose. This would make the code more consistent with the changes made throughout the PR.- [FromQuery] string projectName, + [FromQuery] string projectCode,
59-60: Error message could be updated for consistency.Since we're now validating using
ProjectCode(), consider updating the error message to reference "project code" instead of "project name" for consistency.if (!CrdtProjectsService.ProjectCode().IsMatch(projectName)) - return Results.BadRequest("Project name is invalid"); + return Results.BadRequest("Project code is invalid");backend/FwLite/LcmCrdt/CrdtProjectsService.cs (4)
58-62: Handle potential duplicate matches inGetProject(string code).
UsingFirstOrDefaultwill silently pick the first match if multiple files share the same code. UsingSingleOrDefaultor throwing if multiple matches exist might prevent unexpected ambiguity.
65-68: Validate singleton byGuid.
If two or more entries coincidentally map to the sameGuid, the current method will return the first. Consider throwing an exception if more than one project shares the same ID.
70-72: Consider performance optimization forProjectExists.
SinceProjectExistscallsGetProject, it must scan the entire folder each time. If called frequently, you may want to cache or maintain an index of projects.
96-98: Improve user feedback for invalid code.
You are throwing an error if the code does not match the regex, but consider clarifying the valid regex pattern or advising the user how to fix it.
🛑 Comments failed to post (1)
backend/FwLite/LcmCrdt/CrdtProjectsService.cs (1)
104-105:
⚠️ Potential issuePrevent possible path injection.
Path.GetFileNamehelps mitigate directory traversal, but user-provided codes might still introduce unusual file names. Validate thoroughly or sanitize further to guard against malicious input.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs (1)
69-69: Ensure clarity between name and code parameters.Currently,
_projectNameis passed twice (as both name and code). This may obscure test coverage for scenarios where the display name and code differ. Consider using a distinct string for the code parameter if you intend to validate name-code uniqueness.backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (1)
62-64: Consider adding code or ID details in the disposal log.Currently, you only log the display name (
projectData.Name) when disposing the scope. Including the project code or ID alongside the name might aid in diagnosing issues where multiple projects share similar names.backend/FwLite/LcmCrdt/CrdtProject.cs (1)
24-30: Validate consistency between documentation and usage.The new doc comments clarify that “Name” is a user-display name and “Code” is unique. However, elsewhere (
CrdtProject.Name) references “code” as “Name.” Ensure both the documentation and property naming align to avoid confusion.backend/FwLite/LcmCrdt/CrdtProjectsService.cs (1)
96-101: Minor naming inconsistency in error message.The variable
nameIsInvalidnow refers to a code validation error, not a name validation error.- var nameIsInvalid = $"Project code '{request.Code}' is invalid"; - activity?.SetStatus(ActivityStatusCode.Error, nameIsInvalid); - throw new InvalidOperationException(nameIsInvalid); + var codeIsInvalid = $"Project code '{request.Code}' is invalid"; + activity?.SetStatus(ActivityStatusCode.Error, codeIsInvalid); + throw new InvalidOperationException(codeIsInvalid);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
backend/FwHeadless/Services/SyncHostedService.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/Fixtures/Sena3SyncFixture.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs(1 hunks)backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs(1 hunks)backend/FwLite/FwLiteProjectSync/Program.cs(2 hunks)backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs(7 hunks)backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs(1 hunks)backend/FwLite/FwLiteShared/Sync/BackgroundSyncService.cs(1 hunks)backend/FwLite/FwLiteWeb/Routes/ProjectRoutes.cs(2 hunks)backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/OpenProjectTests.cs(2 hunks)backend/FwLite/LcmCrdt/CrdtProject.cs(2 hunks)backend/FwLite/LcmCrdt/CrdtProjectsService.cs(5 hunks)backend/FwLite/LcmCrdt/CurrentProjectService.cs(1 hunks)backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.Designer.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs(1 hunks)backend/FwLite/LcmCrdt/RemoteSync/CrdtHttpSyncService.cs(1 hunks)backend/FwLite/MiniLcm/Models/ProjectIdentifier.cs(1 hunks)frontend/viewer/src/App.svelte(1 hunks)frontend/viewer/src/DotnetProjectView.svelte(1 hunks)frontend/viewer/src/home/HomeView.svelte(3 hunks)frontend/viewer/src/home/ProjectTitle.svelte(1 hunks)frontend/viewer/src/home/Server.svelte(5 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/ICombinedProjectsService.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/IProjectModel.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectServicesProvider.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectData.ts(1 hunks)frontend/viewer/src/lib/services/projects-service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (26)
- backend/FwLite/MiniLcm/Models/ProjectIdentifier.cs
- frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/IProjectModel.ts
- frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectData.ts
- frontend/viewer/src/home/ProjectTitle.svelte
- backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt
- backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs
- backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.cs
- backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs
- backend/FwLite/FwLiteProjectSync/Program.cs
- backend/FwLite/FwLiteShared/Sync/BackgroundSyncService.cs
- backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs
- backend/FwHeadless/Services/SyncHostedService.cs
- frontend/viewer/src/lib/services/projects-service.ts
- backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs
- backend/FwLite/LcmCrdt/RemoteSync/CrdtHttpSyncService.cs
- backend/FwLite/LcmCrdt/CurrentProjectService.cs
- frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/ICombinedProjectsService.ts
- backend/FwLite/LcmCrdt.Tests/OpenProjectTests.cs
- backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs
- frontend/viewer/src/home/Server.svelte
- frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectServicesProvider.ts
- backend/FwLite/FwLiteWeb/Routes/ProjectRoutes.cs
- frontend/viewer/src/home/HomeView.svelte
- frontend/viewer/src/DotnetProjectView.svelte
- frontend/viewer/src/App.svelte
- backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Build API / publish-api
- GitHub Check: Analyze (csharp)
- GitHub Check: Build FW Lite and run tests
🔇 Additional comments (18)
backend/FwLite/FwLiteProjectSync.Tests/Fixtures/Sena3SyncFixture.cs (1)
57-59: Update to CreateProject call using the new API signature.The code correctly adopts the new API signature that separates project code and name. The change is consistent with the PR objective of using more explicit identifiers for projects.
For test clarity, consider using different values for code and name to verify both fields are properly stored and retrieved. Additionally, adding a comment explaining the parameter purpose would improve readability.
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (2)
41-46: Confirm exception strategy for missing project data.Throwing
InvalidOperationExceptionis acceptable, but consider whether a more specific exception type or a user-friendly error handling strategy might be warranted. This helps upstream callers handle missing project data gracefully.
49-54: Switch to Guid-based project lookup looks good.Changing from a string-based lookup to a Guid-based lookup better enforces unique identification and prevents naming collisions. This approach aligns well with the broader transition to code-based identification.
backend/FwLite/LcmCrdt/CrdtProjectsService.cs (12)
22-24: Approved: Method signature update to use 'code' parameter.Changed from using
nametocodefor project identification, which aligns with the PR's objective to improve project loading by using more consistent identifiers.
53-55: Approved: Updated project creation to use code instead of name.The
ListProjectsmethod now correctly uses the file name without extension as the project code, improving consistency in project identification.
58-63: Approved: GetProject method updated to use code parameter.Method is now consistently using the project code for identification rather than name.
65-68: Added method to get project by GUID.This new overload is essential for the PR's objective of using GUIDs for routing purposes.
70-73: Approved: ProjectExists method updated to use code.Consistent usage of
codefor project identification.
76-77: Approved: Added Code property to CreateProjectRequest.The record now stores both Name and Code, allowing the project name to be displayed while using code for file naming.
104-105: Approved: Updated file path construction to use code.Consistently using code for SQLite file naming improves reliability of file operations.
113-113: Approved: Updated CrdtProject construction.Consistent usage of code for project identification.
120-124: Approved: Updated ProjectData construction to include code.Now storing both the project name (for display) and code (for file naming), which aligns with the PR objective.
171-173: Approved: DeleteProject method now uses GUID for project identification.This change is consistent with using more reliable identifiers for project operations.
195-196: Approved: Renamed regex method to ProjectCode.Properly renamed to reflect its usage for validating project codes rather than names.
89-89:✅ Verification successful
Verify the behavior of CreateExampleProject.
The implementation uses the same value for both name and code. Confirm this is intentional for example projects.
🏁 Script executed:
#!/bin/bash # Check if there are other places in the codebase that call CreateExampleProject rg "CreateExampleProject\(" -A 2 -B 2Length of output: 2360
Verified: The behavior in
CreateExampleProject—specifically, passing the same value for both the project name and code—is consistent with its usage throughout the codebase. Our search confirmed that this pattern is also employed in the test cases (e.g., inbackend/FwLite/LcmCrdt.Tests/OpenProjectTests.cs) and in higher-level services (e.g., inCombinedProjectsService). This indicates that the design choice for example projects is intentional.backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.Designer.cs (3)
14-16: Approved: New migration to add Code column to ProjectData.The migration properly identifies the context and provides a descriptive name.
33-35: Approved: Code column configuration.The Code column is properly configured as a required text field, which matches the project requirements for a reliable file naming convention.
24-56:✅ Verification successful
Verify the migration strategy for existing data.
The migration adds a new required
Codecolumn to theProjectDatatable. Ensure there's a corresponding migration file (not just the Designer) that handles populating this column for existing records, likely defaulting to the existingNamevalues.
🏁 Script executed:
#!/bin/bash # Look for the corresponding migration file fd --type f "20250306152556_AddCodeToProjectData.cs" --exec cat {}Length of output: 1080
Migration strategy verified: The migration file properly updates the new required
Codecolumn.The migration file first adds the column as nullable, then copies existing values from
Nameusing an SQL update, and finally altersCodeto be non-nullable. This confirms that the migration strategy for existing data is correctly implemented.
| public class CrdtProject(string code, string dbPath) : IProjectIdentifier | ||
| { | ||
| public CrdtProject(string name, string dbPath, IMemoryCache memoryCache) : this(name, dbPath) | ||
| public CrdtProject(string code, string dbPath, IMemoryCache memoryCache) : this(code, dbPath) | ||
| { | ||
| Data = CurrentProjectService.LookupProjectData(memoryCache, this); | ||
| } | ||
|
|
||
| public string Name { get; } = name; | ||
| /// <summary> | ||
| /// Actually the Lexbox project code, not the name | ||
| /// </summary> | ||
| public string Name { get; } = code; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename property to reduce confusion between “Name” and “Code”.
Declaring public string Name { get; } = code; while calling it “code” introduces ambiguity. Renaming the property or using two properties for “Name” and “Code” can help maintain clarity throughout the codebase.
myieye
force-pushed
the
bug/1525-using-code-instead-of-name-broke-existing-projects
branch
from
cf66405 to
1e207a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (1)
41-46: Consider using a more descriptive exception or a unified error-handling approach.
Currently,InvalidOperationExceptionis thrown when the project is not found or the project data is missing. While this is not incorrect, you may want to use a dedicated exception (e.g., a “NotFoundException” or similarly specific) or unify error handling for missing projects/data throughout the codebase.backend/FwLite/LcmCrdt/CrdtProjectsService.cs (3)
58-63: Optional: Consider adding case-insensitive matching.
If project codes are meant to be case-insensitive, convert both filename and code to a common casing before comparison. Otherwise, this is fine.
65-68: Beware of potential performance overhead.
GetProject(Guid id)enumerates all projects and checks their data for a matching ID. This is acceptable for smaller repositories but may not scale well for larger datasets. A more direct lookup or caching layer could improve performance.
89-90: Minor clarity concern: setting bothNameandCodeto the same value.
While this is likely intended for creating example projects, it can be slightly confusing. Consider clarifying the rationale in a comment or naming it differently for demos.backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.Designer.cs (1)
33-35: Consider adding an index (or unique constraint) to theCodecolumn if it must be used as a key.
TheCodeproperty is defined as required, which ensures no null entries. If it’s intended for lookups or uniqueness, consider adding an index or unique constraint to improve performance and data integrity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
backend/FwHeadless/Services/SyncHostedService.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/Fixtures/Sena3SyncFixture.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs(1 hunks)backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs(1 hunks)backend/FwLite/FwLiteProjectSync/Program.cs(2 hunks)backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs(7 hunks)backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs(1 hunks)backend/FwLite/FwLiteShared/Sync/BackgroundSyncService.cs(1 hunks)backend/FwLite/FwLiteWeb/Routes/ProjectRoutes.cs(2 hunks)backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt(1 hunks)backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/OpenProjectTests.cs(2 hunks)backend/FwLite/LcmCrdt/CrdtProject.cs(2 hunks)backend/FwLite/LcmCrdt/CrdtProjectsService.cs(5 hunks)backend/FwLite/LcmCrdt/CurrentProjectService.cs(1 hunks)backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.Designer.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.cs(1 hunks)backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs(1 hunks)backend/FwLite/LcmCrdt/RemoteSync/CrdtHttpSyncService.cs(1 hunks)backend/FwLite/MiniLcm/Models/ProjectIdentifier.cs(1 hunks)frontend/viewer/src/App.svelte(1 hunks)frontend/viewer/src/DotnetProjectView.svelte(1 hunks)frontend/viewer/src/home/HomeView.svelte(3 hunks)frontend/viewer/src/home/ProjectTitle.svelte(1 hunks)frontend/viewer/src/home/Server.svelte(5 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/ICombinedProjectsService.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/IProjectModel.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectServicesProvider.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectData.ts(1 hunks)frontend/viewer/src/lib/services/projects-service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (28)
- backend/FwLite/MiniLcm/Models/ProjectIdentifier.cs
- frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectData.ts
- backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs
- backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs
- frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/IProjectModel.ts
- backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs
- backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs
- backend/FwLite/FwLiteShared/Sync/BackgroundSyncService.cs
- backend/FwLite/LcmCrdt/RemoteSync/CrdtHttpSyncService.cs
- frontend/viewer/src/home/Server.svelte
- frontend/viewer/src/App.svelte
- backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt
- backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs
- frontend/viewer/src/home/HomeView.svelte
- backend/FwLite/LcmCrdt.Tests/OpenProjectTests.cs
- backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.cs
- backend/FwLite/LcmCrdt/CurrentProjectService.cs
- frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/ICombinedProjectsService.ts
- frontend/viewer/src/home/ProjectTitle.svelte
- backend/FwLite/FwLiteProjectSync.Tests/Fixtures/Sena3SyncFixture.cs
- backend/FwHeadless/Services/SyncHostedService.cs
- backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs
- frontend/viewer/src/lib/services/projects-service.ts
- backend/FwLite/FwLiteWeb/Routes/ProjectRoutes.cs
- frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectServicesProvider.ts
- frontend/viewer/src/DotnetProjectView.svelte
- backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs
- backend/FwLite/FwLiteProjectSync/Program.cs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (14)
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (1)
48-49: Verify correctness of usingprojectData.Nameinstead ofprojectData.Code.
You log and dispose the project scope under the name property, but elsewhere we've seen that "Name" might store a display name vs. a code. If your intention is to track the display name in logs, then this is fine. Otherwise, consider verifying that you are logging the correct property.Also applies to: 53-54, 62-62, 64-64
backend/FwLite/LcmCrdt/CrdtProject.cs (2)
5-16: Rename property to avoid confusion between “Name” and “Code”.
This class setspublic string Name { get; } = code;, which conflicts with the typical understanding of “Name” vs. “Code”. Retaining two distinct properties could help maintain clarity throughout the codebase.
24-25: Documentation updates look good.
Specifying that “Name” is the display name and “Code” is the unique code helps clarify the usage.Also applies to: 30-30
backend/FwLite/LcmCrdt/CrdtProjectsService.cs (7)
22-25: New parameter name aligns with the shift to codes.
Switching fromnametocodeinIProjectProvider.GetProjectis consistent with the new approach.
53-54: Check for potential filename collisions when deriving the code.
UsingPath.GetFileNameWithoutExtension(file)as the project code can lead to collisions if multiple files share the same name or if case sensitivity comes into play. Consider verifying uniqueness or handling conflicts.
70-73: Existence check is straightforward.
This is a clean, direct approach for checking a project's existence.
96-97: Regex usage for code validation is appropriate.
Validate that allowed characters match your project requirements. Otherwise, this is fine.Also applies to: 98-98
120-121: ProjectData initialization aligns with the new code parameter.
No issues here—this is consistent with the “display name” plus “unique code” usage.
171-173: Deleting projects by GUID is consistent.
Shifting from name-based to GUID-based deletion is in line with the rest of these changes.
195-197: Regex rename to “ProjectCode” is coherent.
This partial method name helps differentiate from the older “ProjectName” usage and maintains clarity.backend/FwLite/LcmCrdt/Migrations/20250306152556_AddCodeToProjectData.Designer.cs (4)
15-16: Migration class naming looks valid.
The migration class name and attributes (DbContext, Migration) follow conventional EF Core naming standards and appear consistent with the rest of the codebase.
24-56: Entity mapping forProjectDatalooks correct.
The mapping of properties to columns (Id, ClientId, Code, Name, etc.) is properly structured for EF Core. They align with typical guidelines for table naming and property definition.
46-48: Confirm backward compatibility for existingNameusage.
TheNameproperty is also marked as required. Ensure that any existing migrations or data rows have non-null values for this in older records or that a default migration strategy merges Name and Code appropriately.
57-639: Remaining entity definitions and relationships are standard EF Core auto-generated code.
No issues identified across the remaining model definitions, foreign key relationships, and table mappings. These appear to be typical EF Core migrations with coherent indexing strategies on related entities.
Resolves #1525
With this PR, in most cases we now load crdt projects by ID. So that we can always find them regardless of whether the name or code was used for the sqlite file name.
Also, we now save both the project code and name in ProjectData, so that we can use the code as a reliably safe file name, but still display the name.
If the name and code are identical we only show one.
For routing I now use the guid, which means the name isn't automatically available to display. Initially, I just added the name as a query-param, but felt that was dissatisfying. It was much simpler, so maybe I should have left it 🤷.
Summary by CodeRabbit
New Features
Refactor
Codein project data structures to enhance project identification.