fix: 适应区块新方案 #25
fix: 适应区块新方案 #25
Conversation
WalkthroughThe changes in the Changes
Sequence DiagramsequenceDiagram
participant Client
participant BlockController
participant Database
Client->>BlockController: Request block build
BlockController->>BlockController: Prepare historyParam
BlockController->>Database: Update block history
BlockController->>Database: Update block assets
Database-->>BlockController: Return updated block
BlockController-->>Client: Send block build response
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🔭 Outside diff range comments (1)
app/controller/material-center/block.ts (1)
Line range hint
245-249: Add validation for 'label' and 'framework' in 'ensureBlockId' methodIn the
ensureBlockIdmethod, ifidis not provided, the code relies onlabelandframeworkfromblockDatato find or create a block. Iflabelorframeworkisundefined, it may cause errors during database operations.Apply this diff to add validation:
+ if (!label || !framework) { + throwApiError('Missing required block data', Number(E_ErrorCode.BadRequest), E_MaterialErrorCode.CM204); + }
| const { data: oldBlock } = await materialCenter.block.findById(block.id); | ||
| block.last_build_info = build_info; | ||
| block.assets = assets; | ||
| const blockResult = await materialCenter.block.update({ | ||
| ...block, | ||
| histories: [...oldBlock.histories.map(({ id }) => id), history.data.id] | ||
| }); | ||
| this.ctx.body = blockResult; |
There was a problem hiding this comment.
Ensure 'oldBlock.histories' is defined before mapping
In line 238, the code assumes that oldBlock.histories is defined when mapping to extract ids. If oldBlock.histories is undefined, this will cause a runtime error.
Apply this diff to handle undefined histories:
- histories: [...oldBlock.histories.map(({ id }) => id), history.data.id]
+ const existingHistories = oldBlock.histories ? oldBlock.histories.map(({ id }) => id) : [];
+ histories: [...existingHistories, history.data.id]📝 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.
| const { data: oldBlock } = await materialCenter.block.findById(block.id); | |
| block.last_build_info = build_info; | |
| block.assets = assets; | |
| const blockResult = await materialCenter.block.update({ | |
| ...block, | |
| histories: [...oldBlock.histories.map(({ id }) => id), history.data.id] | |
| }); | |
| this.ctx.body = blockResult; | |
| const { data: oldBlock } = await materialCenter.block.findById(block.id); | |
| block.last_build_info = build_info; | |
| block.assets = assets; | |
| const blockResult = await materialCenter.block.update({ | |
| ...block, | |
| const existingHistories = oldBlock.histories ? oldBlock.histories.map(({ id }) => id) : []; | |
| histories: [...existingHistories, history.data.id] | |
| }); | |
| this.ctx.body = blockResult; |
| const { deploy_info: message, block, version } = this.ctx.request.body; | ||
| const { materialCenter } = this.ctx.service; | ||
| const { content } = block; |
There was a problem hiding this comment.
Add validation for 'block' and 'version' in the 'build' method
The block and version parameters are destructured from this.ctx.request.body without validation. If either block or version is undefined, it could lead to runtime errors when accessing their properties.
Apply this diff to add validation:
+ this.ctx.validate({
+ block: 'object',
+ version: 'string',
+ }, this.ctx.request.body);📝 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.
| const { deploy_info: message, block, version } = this.ctx.request.body; | |
| const { materialCenter } = this.ctx.service; | |
| const { content } = block; | |
| this.ctx.validate({ | |
| block: 'object', | |
| version: 'string', | |
| }, this.ctx.request.body); | |
| const { deploy_info: message, block, version } = this.ctx.request.body; | |
| const { materialCenter } = this.ctx.service; | |
| const { content } = block; |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controller/material-center/block.ts(9 hunks)
🔇 Additional comments (6)
app/controller/material-center/block.ts (6)
13-15: LGTM! Import statements are well organized.The consolidation of related imports improves code organization.
32-32: LGTM! Clean destructuring of query parameters.The simplified destructuring maintains functionality while improving readability.
54-56: LGTM! Validation object formatting is clean.The reformatted validation object improves readability while maintaining the same validation logic.
205-206:⚠️ Potential issueDocument the breaking change in build behavior.
The change from executing builds to direct database updates is a significant breaking change that needs to be documented. This could affect downstream systems expecting the build process to occur.
Please add documentation explaining:
- Why the build execution was removed
- Impact on dependent systems
- Migration guide for affected users
222-223: Clarify the empty screenshot handling.The code sets
block.screenshot = ''earlier but still includes it in historyParam. This seems inconsistent.
208-216: Verify the hardcoded build results.The code sets
build_info.resultto true andassetsto empty arrays without any actual build process or validation.
| const { data: oldBlock } = await materialCenter.block.findById(block.id); | ||
| block.last_build_info = build_info; | ||
| block.assets = assets; | ||
| block.current_history = history.data.id; | ||
| const blockResult = await materialCenter.block.update({ | ||
| ...block, | ||
| histories: [...oldBlock.histories.map(({ id }) => id), history.data.id] | ||
| }); |
There was a problem hiding this comment.
Add null checks and error handling for database operations.
The code assumes several operations will succeed without proper error handling:
- Accessing oldBlock.histories without null check (previously flagged issue)
- No error handling for blockHistory.create
- No error handling for block.update
Apply this diff to add proper error handling:
- const history = await materialCenter.blockHistory.create(historyParam);
- // 更新区块
- const { data: oldBlock } = await materialCenter.block.findById(block.id);
- block.last_build_info = build_info;
- block.assets = assets;
- block.current_history = history.data.id;
- const blockResult = await materialCenter.block.update({
- ...block,
- histories: [...oldBlock.histories.map(({ id }) => id), history.data.id]
- });
+ try {
+ const history = await materialCenter.blockHistory.create(historyParam);
+ if (!history?.data?.id) {
+ throwApiError('Failed to create block history', Number(E_ErrorCode.BadRequest));
+ }
+
+ // 更新区块
+ const { data: oldBlock } = await materialCenter.block.findById(block.id);
+ if (!oldBlock) {
+ throwApiError('Block not found', Number(E_ErrorCode.BadRequest));
+ }
+
+ block.last_build_info = build_info;
+ block.assets = assets;
+ block.current_history = history.data.id;
+
+ const existingHistories = oldBlock.histories?.map(({ id }) => id) ?? [];
+ const blockResult = await materialCenter.block.update({
+ ...block,
+ histories: [...existingHistories, history.data.id]
+ });
+
+ if (!blockResult?.data) {
+ throwApiError('Failed to update block', Number(E_ErrorCode.BadRequest));
+ }
+
+ return blockResult;
+ } catch (error) {
+ throwApiError(
+ `Failed to process block: ${error.message}`,
+ Number(E_ErrorCode.BadRequest)
+ );
+ }📝 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.
| const { data: oldBlock } = await materialCenter.block.findById(block.id); | |
| block.last_build_info = build_info; | |
| block.assets = assets; | |
| block.current_history = history.data.id; | |
| const blockResult = await materialCenter.block.update({ | |
| ...block, | |
| histories: [...oldBlock.histories.map(({ id }) => id), history.data.id] | |
| }); | |
| try { | |
| const history = await materialCenter.blockHistory.create(historyParam); | |
| if (!history?.data?.id) { | |
| throwApiError('Failed to create block history', Number(E_ErrorCode.BadRequest)); | |
| } | |
| // 更新区块 | |
| const { data: oldBlock } = await materialCenter.block.findById(block.id); | |
| if (!oldBlock) { | |
| throwApiError('Block not found', Number(E_ErrorCode.BadRequest)); | |
| } | |
| block.last_build_info = build_info; | |
| block.assets = assets; | |
| block.current_history = history.data.id; | |
| const existingHistories = oldBlock.histories?.map(({ id }) => id) ?? []; | |
| const blockResult = await materialCenter.block.update({ | |
| ...block, | |
| histories: [...existingHistories, history.data.id] | |
| }); | |
| if (!blockResult?.data) { | |
| throwApiError('Failed to update block', Number(E_ErrorCode.BadRequest)); | |
| } | |
| return blockResult; | |
| } catch (error) { | |
| throwApiError( | |
| `Failed to process block: ${error.message}`, | |
| Number(E_ErrorCode.BadRequest) | |
| ); | |
| } |
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit