New #79
New #79
Conversation
WalkthroughThis update implements full CRUD operations for the Artist entity in the API controller, enforces authorization, and connects the UI to fetch and display real artist data from the backend. It also adds validation and default deserialization for the Artist struct and improves error handling and table rendering in the UI. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ArtistPage (UI)
participant Controller as Controller
participant API as ArtistController (API)
participant Repo as ArtistRepository
UI->>Controller: Initialize Controller
Controller->>API: Fetch artist summaries (query)
API->>Repo: Query artists
Repo-->>API: Return artist list
API-->>Controller: Return artist summaries
Controller-->>UI: Provide artist data Resource
UI->>Controller: Create artist (input fields)
Controller->>API: Create artist (ArtistCreateRequest, Authorization)
API->>Repo: Insert new artist
Repo-->>API: Return created artist
API-->>Controller: Return created artist
Controller-->>UI: Update UI with new artist
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
@RyanCho-0 could you please help check this conflict from rust-sdk nothing seems to be wrong on my locals |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
packages/build-ui/src/pages/agits/_id/management/artists/controllers.rs (2)
137-144: 🛠️ Refactor suggestionCreate artist success handling lacks data refresh.
After successfully creating an artist, the data isn't refreshed, which means the UI won't show the newly created artist until the page is reloaded.
Update the success handler to refresh the artist data:
match res { Ok(_) => { btracing::info!("Artist created successfully"); + // If using the refreshable resource pattern suggested earlier + ctrl.refresh_artists(); } Err(e) => { btracing::error!("Error creating artist: {:?}", e); } };
147-166: 🛠️ Refactor suggestionRemove artist method lacks data refresh mechanism.
Similar to the create method, the remove_artist method doesn't refresh the artist data after a successful operation.
Add a data refresh after successful artist removal:
match res { Ok(_) => { btracing::info!("Artist removed successfully"); + // If using the refreshable resource pattern suggested earlier + // Need to capture and clone the controller here + let ctrl = self.clone(); + spawn_local(async move { + ctrl.refresh_artists(); + }); } Err(e) => { btracing::error!("Error removing artist: {:?}", e); } };
🧹 Nitpick comments (2)
packages/build-ui/src/pages/agits/_id/management/artists/controllers.rs (2)
135-136: Debug log messages don't match field semantics.The debug logs refer to
social_media, but these don't match the semantic meaning of the fields they're logging:
social_mediavaluesocial_mediais actually storingmediumvalueUpdate the debug log messages to match the actual field semantics:
- tracing::debug!("mail: {:?}", artist_inputs.social_media); 8000 - tracing::debug!("social_media: {:?}", artist_inputs.medium); + tracing::debug!("social_media (for mail field): {:?}", artist_inputs.social_media); + tracing::debug!("medium (for social_media field): {:?}", artist_inputs.medium);
199-201: Incomplete error message in field update error handler.The error message for invalid field names is incomplete ("invalid ..."), which doesn't provide useful information for debugging.
Improve the error message to be more descriptive:
_ => { - btracing::error!("{} {}", self.lang, "invalid ...") + btracing::error!("Invalid field name: '{}' with value '{}'", field, value) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
deps/rust-sdk(1 hunks)packages/api/src/controllers/v1/artist/mod.rs(4 hunks)packages/api/src/controllers/v1/collection/mod.rs(1 hunks)packages/build-ui/src/pages/agits/_id/management/artists/controllers.rs(4 hunks)packages/build-ui/src/pages/agits/_id/management/artists/page.rs(9 hunks)packages/common/src/tables/artists/artists.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/api/src/controllers/v1/artist/mod.rs (3)
packages/api/src/controllers/v1/collection/mod.rs (3)
create(88-136)update(138-150)delete(152-161)packages/api/src/controllers/v1/agit/mod.rs (3)
create(169-197)update(199-211)delete(213-220)packages/api/src/controllers/v1/collector/mod.rs (3)
create(95-139)update(141-152)delete(154-162)
🔇 Additional comments (17)
deps/rust-sdk (1)
1-1:❓ Verification inconclusive
Ensure submodule pointer is up-to-date and tested
Therust-sdksubproject reference has been bumped to commit73463faf4ab5859caa7568496caeeb95dc7f5369. Please confirm that this version includes the required artist CRUD support, update any CI submodule initialization steps, and run integration tests to catch breaking changes.Run something like:
🏁 Script executed:
#!/bin/bash # Verify the submodule commit git submodule status deps/rust-sdk | grep '73463faf4ab5859caa7568496caeeb95dc7f5369' # Then exercise the new SDK endpoints: cd packages/api && cargo test cd ../build-ui && npm testLength of output: 388
Ensure
rust-sdksubmodule is up-to-date and integration tests pass
Therust-sdksubmodule is correctly pinned to commit73463faf4ab5859caa7568496caeeb95dc7f5369, but sandbox executions showed missing tooling and no UI tests. Please manually verify the following before merging:
- Confirm the submodule pointer:
git submodule status deps/rust-sdk | grep 73463faf4ab5859caa7568496caeeb95dc7f5369- Verify that the SDK at this commit includes the required artist CRUD endpoints.
- Update CI steps to initialize submodules before building:
git submodule update --init --recursive- In a Rust/Cargo-enabled environment, run API integration tests:
cd packages/api && cargo test- Add or implement proper UI tests in
build-uiso thatnpm testpasses (currently “no test specified”).packages/common/src/tables/artists/artists.rs (1)
2-2: Good improvement for validation and serialization!Adding validation with
#[derive(validator::Validate)]enables field validation capabilities on theArtiststruct, while#[serde(default)]ensures that missing fields use default values during deserialization. These are important additions for robust API interactions.Also applies to: 4-4
packages/api/src/controllers/v1/collection/mod.rs (1)
131-131: Added missing commaGood fix - this missing comma at the end of the parameter list would have caused compile errors.
packages/api/src/controllers/v1/artist/mod.rs (6)
13-13: Good addition of ServiceError importAdding the ServiceError import is necessary for the error handling in the new CRUD operations.
43-82: Well-implemented create methodThe create method follows the project pattern for entity creation with proper:
- Authorization checking
- Structured parameter extraction
- Repository interaction
- Error handling
- Response formatting
This implementation aligns well with other controllers in the codebase.
84-95: Clean update implementationThe update method properly checks authorization, uses the repository to update the entity, and returns appropriate responses. The pattern matches other controllers in the system.
96-104: Properly implemented delete methodThe delete implementation includes authorization verification and maps repository errors to appropriate service errors, following the established pattern in other controllers.
132-134: Correctly wired create actionThe act method now correctly forwards to the create implementation with appropriate parameter passing.
147-150: Well connected update and delete actionsThe act_by_id method now calls the appropriate implementation methods for update and delete operations, passing all necessary parameters.
Also applies to: 152-154
packages/build-ui/src/pages/agits/_id/management/artists/page.rs (5)
16-23: Good async data handling implementationThe code now properly handles artist data as a Result type, with a derived signal for total count that gracefully handles error states. This pattern enables proper error handling throughout the UI.
27-30: Improved responsive layoutThe container now has a maximum width constraint of 1800px which helps with wide screen displays, and the total count display provides users with useful information about the data.
48-49: Better table container stylingAdding overflow-x-auto to the container ensures the table can be scrolled horizontally on smaller screens, improving mobile usability.
52-115: Good table column width distributionExplicitly setting width classes for all table columns ensures consistent layout across different screen sizes and data lengths. The widths are appropriately distributed based on content needs.
127-214: Comprehensive state handlingThe UI now properly handles three distinct states:
- Empty state with helpful user guidance
- Data display with proper iteration through items
- Error state with user-friendly error message
This ensures a robust user experience across all possible data scenarios.
packages/build-ui/src/pages/agits/_id/management/artists/controllers.rs (3)
12-14: Import statements expanded properly for new functionality.The updated imports from
common::tables::preludenow correctly include all the types needed for the new artist management functionality (ArtistByIdAction, ArtistCreateRequest, etc.).
18-18: Resource type import added for asynchronous data handling.Good addition of the QueryResponse type from bdk::prelude, which is necessary for the new asynchronous data fetching approach.
32-32: Improved data fetching architecture with Resource type.Changing the artist field from a simple Signal to a Resource is an excellent architectural improvement. This change enables proper asynchronous data fetching from the server rather than relying on hardcoded data.
packages/build-ui/src/pages/agits/_id/management/artists/controllers.rs
Outdated
Show resolved
Hide resolved
| .act(ArtistAction::Create( | ||
| ArtistCreateRequest { | ||
| title: artist_inputs.display_name, | ||
| title: artist_inputs.display_name.clone(), | ||
| mail: artist_inputs.social_media.clone(), | ||
| social_media: artist_inputs.medium.clone(), | ||
| intro: artist_inputs.theme, | ||
| biography: artist_inputs.art_style, | ||
| revenue: todo!(), | ||
| attributes_type: todo!(), | ||
| featured_work: todo!(), | ||
| artworks: todo!(), | ||
| name: todo!(), | ||
| status: todo!(), | ||
| revenue: 6.70, | ||
| attributes_type: vec!["Paid".to_string(), "Verified".to_string()], | ||
| featured_work: "featured_work".to_string(), | ||
| artworks: 100, | ||
| name: artist_inputs.display_name.clone(), | ||
| status: "Active".to_string(), | ||
| }, |
There was a problem hiding this comment.
Field mapping in create_artist seems counterintuitive.
The field mapping in the ArtistCreateRequest doesn't match the expected semantics:
titleis set todisplay_namemailis set tosocial_mediasocial_mediais set tomedium
This suggests a misunderstanding of field purposes or incorrect mapping.
Consider reviewing the field mappings to ensure they correctly match the intended data:
ArtistCreateRequest {
title: artist_inputs.display_name.clone(),
- mail: artist_inputs.social_media.clone(),
- social_media: artist_inputs.medium.clone(),
+ mail: artist_inputs.social_media.clone(), // Consider renaming the field to email_address for clarity
+ social_media: artist_inputs.social_media.clone(),
intro: artist_inputs.theme,
biography: artist_inputs.art_style,
revenue: 6.70,
attributes_type: vec!["Paid".to_string(), "Verified".to_string()],
featured_work: "featured_work".to_string(),
artworks: 100,
name: artist_inputs.display_name.clone(),
status: "Active".to_string(),
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/build-ui/src/pages/agits/_id/management/artists/controllers.rs
around lines 119 to 132, the fields in ArtistCreateRequest are incorrectly
mapped, with title assigned display_name, mail assigned social_media, and
social_media assigned medium. Review the intended purpose of each field and
update the assignments so that title corresponds to the artist's title or name,
mail corresponds to the artist's email, and social_media corresponds to the
correct social media handle or link, ensuring each field accurately reflects its
semantic meaning.
| revenue: 6.70, | ||
| attributes_type: vec!["Paid".to_string(), "Verified".to_string()], | ||
| featured_work: "featured_work".to_string(), | ||
| artworks: 100, | ||
| name: artist_inputs.display_name.clone(), | ||
| status: "Active".to_string(), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded values in create_artist.
Several fields in the ArtistCreateRequest are hardcoded rather than derived from user input:
- revenue: 6.70
- attributes_type: vec!["Paid".to_string(), "Verified".to_string()]
- featured_work: "featured_work".to_string()
- artworks: 100
- status: "Active".to_string()
These hardcoded values should either be:
- Added to the ArtistInputField struct if they need to be user-editable
- Made configurable through application settings if they're fixed but might change
- Documented with comments if they're intended to be fixed values
Example for option 1:
// In ArtistInputField struct definition
pub struct ArtistInputField {
display_name: String,
social_media: String,
medium: String,
theme: String,
art_style: String,
introduction: String,
biography: String,
+ revenue: f64,
+ status: String,
+ featured_work: String,
}
// Then update the create_artist method
ArtistCreateRequest {
// ...
- revenue: 6.70,
+ revenue: artist_inputs.revenue,
// ...
- featured_work: "featured_work".to_string(),
+ featured_work: artist_inputs.featured_work,
// ...
- status: "Active".to_string(),
+ status: artist_inputs.status,
}🤖 Prompt for AI Agents
In packages/build-ui/src/pages/agits/_id/management/artists/controllers.rs
around lines 126 to 132, several fields in the ArtistCreateRequest are hardcoded
instead of using user input. To fix this, either add these fields (revenue,
attributes_type, featured_work, artworks, status) to the ArtistInputField struct
if they should be user-editable, or make them configurable via application
settings if they are fixed but subject to change. If they are intended to be
constant, add clear comments explaining their fixed nature. Update the code
accordingly to replace hardcoded values with the appropriate source.
The remote dev branch contains updates that your local branch is missing. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/api/src/controllers/v1/artist/mod.rs (1)
115-116: Consider more robust error handling for total_count extraction.The current code uses
unwrap_or(0)which silently falls back to 0 if the column extraction fails. Consider adding a warning log when this happens to help detect potential schema issues.-total_count = row.try_get::<i64, _>("total_count").unwrap_or(0); +total_count = match row.try_get::<i64, _>("total_count") { + Ok(count) => count, + Err(e) => { + tracing::warn!("Failed to get total_count from query result: {e}"); + 0 + } +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/api/src/controllers/v1/artist/mod.rs(5 hunks)packages/build-ui/src/pages/agits/_id/management/artists/controllers.rs(5 hunks)packages/build-ui/src/pages/agits/_id/management/artists/new/page.rs(1 hunks)packages/main-ui/src/pages/main/page.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/main-ui/src/pages/main/page.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/build-ui/src/pages/agits/_id/management/artists/controllers.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/build-ui/src/pages/agits/_id/management/artists/new/page.rs (1)
packages/build-ui/src/pages/agits/_id/management/artists/controllers.rs (1)
create_artist(108-139)
packages/api/src/controllers/v1/artist/mod.rs (3)
packages/api/src/controllers/v1/collection/mod.rs (4)
create(88-136)update(138-150)delete(152-161)query(54-86)packages/api/src/controllers/v1/collector/mod.rs (4)
create(95-137)update(139-150)delete(152-160)query(57-93)packages/api/src/controllers/v1/agit/mod.rs (4)
create(169-197)update(199-211)delete(213-220)query(135-167)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: api
- GitHub Check: test
- GitHub Check: main-ui
- GitHub Check: build-ui
🔇 Additional comments (7)
packages/build-ui/src/pages/agits/_id/management/artists/new/page.rs (1)
194-196: Good update to async handler for artist creation.The change correctly updates the click handler to be asynchronous and properly awaits the completion of
ctrl.create_artist(). This ensures the UI waits for the server response before proceeding, providing better user experience and preventing potential race conditions.packages/api/src/controllers/v1/artist/mod.rs (6)
2-3: LGTM: Appropriate imports added to support new functionality.The added imports for
QueryResponse,ServiceError, andPgRowalign with the new CRUD operations being implemented.Also applies to: 13-14
44-83: Well-implemented create method with proper authorization check.The
createmethod follows the established pattern in the codebase, with authorization validation, structured parameter handling, and proper repository interaction. The debug logging is also a good practice.
85-96: LGTM: Update method implementation.The implementation correctly validates authorization, delegates to the repository layer, and handles error responses appropriately.
97-105: LGTM: Delete method with proper error handling.Good implementation that checks authorization and maps repository errors to appropriate service errors.
152-155: LGTM: Act method properly delegating to create.The public
actmethod now correctly delegates to the privatecreatemethod, maintaining the proper authorization context.
167-174: LGTM: Act_by_id method properly delegating to update and delete.The implementation correctly passes the necessary parameters to the private methods and wraps responses appropriately.
| match q { | ||
| ArtistParam::Query(param) => { | ||
| Ok(Json(ArtistGetResponse::Query(ctrl.query(param).await?))) | ||
| } | ||
| } |
There was a problem hiding this comment.
Update list method to pass auth to query.
The list method needs to pass the authorization to the query method to ensure proper access control.
match q {
ArtistParam::Query(param) => {
- Ok(Json(ArtistGetResponse::Query(ctrl.query(param).await?)))
+ Ok(Json(ArtistGetResponse::Query(ctrl.query(claim, param).await?)))
}
}📝 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.
| match q { | |
| ArtistParam::Query(param) => { | |
| Ok(Json(ArtistGetResponse::Query(ctrl.query(param).await?))) | |
| } | |
| } | |
| match q { | |
| ArtistParam::Query(param) => { | |
| Ok(Json(ArtistGetResponse::Query(ctrl.query(claim, param).await?))) | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/api/src/controllers/v1/artist/mod.rs around lines 131 to 135, the
list method calls the query method without passing the authorization
information, which is required for proper access control. Modify the call to
query to include the authorization parameter along with the existing parameters,
ensuring that the authorization context is forwarded correctly.
| ) -> Result<Json<ArtistGetResponse>> { | ||
| match q { | ||
| ArtistParam::Query(param) => { | ||
| Ok(Json(ArtistGetResponse::Query(ctrl.query(param).await?))) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Return type consistency check
The list method returns Json<ArtistGetResponse> while individual operations return Json<Artist>. This is appropriate but ensure the frontend handling accounts for these different response structures.
🏁 Script executed:
#!/bin/bash
# Check how frontend handles different response types
# Look for places where artist API responses are processed
rg -A 5 "ArtistGetResponse" --type rust packages/build-ui/
# Check if there's proper handling for both Query and individual Artist responses
rg -A 8 "match res \{" --type rust packages/build-ui/ | grep -B 4 -A 4 "Artist"Length of output: 1803
Frontend needs to handle ArtistGetResponse::Query
I don’t see any reference to ArtistGetResponse in packages/build-ui/, and your controllers in
src/pages/agits/_id/management/artists/controllers.rs only match on Ok(_) for create/remove. The list endpoint returns a Query(Vec<Artist>) payload, so the UI must be updated to:
- Add or update the controller action that calls the “list” API and awaits a
ArtistGetResponse::Query(items)result. - Match on the
Queryvariant and extract the vector ofArtiststructs. - Pass that list into your template/component (e.g., iterate over it in Askama or your chosen view layer).
Without these changes, the frontend won’t display fetched artist listings.
🤖 Prompt for AI Agents
In packages/api/src/controllers/v1/artist/mod.rs around lines 130 to 136, the
list method returns Json<ArtistGetResponse> with a Query variant containing a
vector of Artist structs, but the frontend does not handle this response type.
To fix this, update or add a controller action in the frontend (likely in
src/pages/agits/_id/management/artists/controllers.rs) that calls the list API,
awaits the ArtistGetResponse::Query result, matches on the Query variant to
extract the vector of Artist structs, and passes this list to the template or
component for rendering the artist listings.
| async fn query(&self, param: ArtistQuery) -> Result<QueryResponse<ArtistSummary>> { | ||
| let mut total_count = 0; | ||
| let items: Vec<ArtistSummary> = Artist::query_builder() | ||
| .limit(param.size()) | ||
| .page(param.page()) | ||
| .query() | ||
| .map(|row: PgRow| { | ||
| use sqlx::Row; | ||
| total_count = row.try_get::<i64, _>("total_count").unwrap_or(0); | ||
| row.into() | ||
| }) | ||
| .fetch_all(&self.pool) | ||
| .await?; | ||
|
|
||
| Ok(QueryResponse { total_count, items }) | ||
| } |
There was a problem hiding this comment.
Missing authorization check in query method.
Unlike other methods in this controller and similar query methods in other controllers, this method doesn't verify that the user is authorized to access the data.
-async fn query(&self, param: ArtistQuery) -> Result<QueryResponse<ArtistSummary>> {
+async fn query(&self, auth: Option<Authorization>, param: ArtistQuery) -> Result<QueryResponse<ArtistSummary>> {
+ if auth.is_none() {
+ return Err(ServiceError::Unauthorized);
+ }
let mut total_count = 0;
let items: Vec<ArtistSummary> = Artist::query_builder()📝 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.
| async fn query(&self, param: ArtistQuery) -> Result<QueryResponse<ArtistSummary>> { | |
| let mut total_count = 0; | |
| let items: Vec<ArtistSummary> = Artist::query_builder() | |
| .limit(param.size()) | |
| .page(param.page()) | |
| .query() | |
| .map(|row: PgRow| { | |
| use sqlx::Row; | |
| total_count = row.try_get::<i64, _>("total_count").unwrap_or(0); | |
| row.into() | |
| }) | |
| .fetch_all(&self.pool) | |
| .await?; | |
| Ok(QueryResponse { total_count, items }) | |
| } | |
| async fn query(&self, auth: Option<Authorization>, param: ArtistQuery) -> Result<QueryResponse<ArtistSummary>> { | |
| if auth.is_none() { | |
| return Err(ServiceError::Unauthorized); | |
| } | |
| let mut total_count = 0; | |
| let items: Vec<ArtistSummary> = Artist::query_builder() | |
| .limit(param.size()) | |
| .page(param.page()) | |
| .query() | |
| .map(|row: PgRow| { | |
| use sqlx::Row; | |
| total_count = row.try_get::<i64, _>("total_count").unwrap_or(0); | |
| row.into() | |
| }) | |
| .fetch_all(&self.pool) | |
| .await?; | |
| Ok(QueryResponse { total_count, items }) | |
| } |
🤖 Prompt for AI Agents
In packages/api/src/controllers/v1/artist/mod.rs around lines 107 to 122, the
query method lacks an authorization check to verify user permissions before
accessing artist data. Add an authorization step at the start of the method to
confirm the current user's rights to perform this query, consistent with other
methods in this controller. This may involve calling an existing authorization
utility or middleware and returning an error if the user is unauthorized.
|
@RyanCho-0 이 PR 은 close 하는 건가요? 확인하시고 머지하거나 클로즈 부탁드립니다. |
Fetched artist over the api
updated the api/v1/artist/mod.rs for some functions
update the artistpage to fit for 1920px width screen
Summary by CodeRabbit
New Features
Improvements