Skip to content

Remove double-pass defineStack hack, unify config composition flow#1024

Merged
hotlong merged 2 commits intomainfrom
copilot/fix-double-pass-hack-merge
Mar 4, 2026
Merged

Remove double-pass defineStack hack, unify config composition flow#1024
hotlong merged 2 commits intomainfrom
copilot/fix-double-pass-hack-merge

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 4, 2026

defineStack() Zod validation strips runtime properties (listViews, actions) from objects, forcing a second composeStacks() pass to restore them. Console config also manually spreads apps/reports from individual sources instead of using composed output.

Config simplification

  • objectstack.config.ts: Remove defineStack() + second composeStacks() pass. Single composeStacks() call produces final config directly.
  • apps/console/objectstack.shared.ts: Same double-pass removal. Replace manual [...crmApps, ...(todoConfig.apps || []), ...] with composed.apps (CRM nav patch applied post-composition). Replace ...(crmConfig.reports || []) with ...(composed.reports || []).

Before:

const composed = composeStacks(allConfigs, { objectConflict: 'override' });
const validated = defineStack({ ...composed } as any);       // strips listViews, actions
export default {
  ...validated,
  objects: composeStacks([                                    // second pass to restore
    { objects: validated.objects || [] },
    ...allConfigs.map(cfg => ({ views: cfg.views, actions: cfg.actions })),
  ]).objects,
};

After:

const composed = composeStacks(allConfigs, { objectConflict: 'override' });
export default sharedConfig;  // single pass, runtime properties preserved

Tests

  • Added conflict detection test (objectConflict: 'error' catches duplicate account)
  • Added prefixed naming coexistence test (account + ks_account with objectConflict: 'error')

Docs

  • Updated ROADMAP.md Phase 1.5 with composeStacks responsibility split: @object-ui/core handles runtime mapping (views→objects, actions→objects); @objectstack/spec handles protocol-level composition.
Original prompt

This section details on the original issue you should resolve

<issue_title>merge 配置未彻底解决、配置 double-pass hack 和对象命名冲突问题待修复</issue_title>
<issue_description>## 问题描述

近期虽然已将 mergeViewsIntoObjectsmergeActionsIntoObjects 相关逻辑从手动 merge 移动到了 @object-ui/corecomposeStacks(),但实际 main/config 代码流和 objects/views/actions 的 runtime 映射,依然停留在 double-pass hack,以及对象名冲突未彻底解决。


1. defineStack() 验证后又二次 composeStacks(),double-pass hack

  • objectstack.config.tsapps/console/objectstack.shared.ts 都有相似的模式:先 compose+validate,再重新 merge views/actions 到 objects;
  • defineStack() 的 Zod parse 会剥掉非标准字段(如 listViews),导致必须二次 merge,显式 hack;
  • 当前已经是 "merge hidden, not removed"(只是换了个位置藏起来);
  • 建议提炼为唯一的数据流入口工具,或从 schema 协议源头支持动态字段;

2. apps 配置仍然有手动结构展开

  • 比如 console 侧 apps 是 [...crmApps, ...(todoConfig.apps || []), ...(kitchenSinkConfig.apps || [])],而不是完全走 composed.apps;
  • 应简化入口逻辑,只用 composed/apps 统一流;

3. Kitchen Sink 对象未改名,objectConflict 仍然只是静默压制冲突

4. composeStacks 工具重复,协议和工具并存

  • ObjectUI 有独立实现 (@object-ui/core),spec/core 层也有相同工具,功能高度重叠;
  • 建议统一为 spec 层提供的 composeStacks,ObjectUI 侧只保留 runtime 映射适配;

建议修复清单

  1. 明确只调用一次 composeStacks 汇总所有数据,避免 double-pass(如 objects/views/actions apps),不再手动二次 merge;
  2. 完全用 composeStacks 输出 apps(去掉手动展开),apps 部分保持和 objects 一致的数据流;
  3. Kitchen Sink example 对象重命名,按照插件隔离原则区分 source,消除 silent override;
  4. 明确 ObjectUI 和 Spec 层 composeStacks 的职责、避免重复实现;
  5. 评估 ObjectSchema/ViewSchema 支持 runtime 字段的方式,减少 hack;
  6. Tests & Docs:针对合并/冲突场景补充分支测试,ROADMAP.md、架构文档同步更新。

</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
objectui Ready Ready Preview, Comment Mar 4, 2026 5:18am
objectui-demo Ready Ready Preview, Comment Mar 4, 2026 5:18am
objectui-storybook Ready Ready Preview, Comment Mar 4, 2026 5:18am

Request Review

- Remove defineStack() validation pass from objectstack.config.ts and
  apps/console/objectstack.shared.ts that stripped runtime properties
  (listViews, actions) from objects, requiring a second composeStacks()
  call to restore them.
- Use composed.apps in console shared config instead of manual spreading
  [...crmApps, ...(todoConfig.apps || []), ...(kitchenSinkConfig.apps || [])]
- Use composed.reports instead of ...(crmConfig.reports || [])
- Apply CRM navigation patch to composed output instead of pre-composing
- Add 2 tests for conflict detection and prefixed naming pattern
- Update ROADMAP.md with composeStacks responsibility split documentation

Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix double-pass hack and object naming conflicts Remove double-pass defineStack hack, unify config composition flow Mar 4, 2026
@hotlong hotlong marked this pull request as ready for review March 4, 2026 05:17
Copilot AI review requested due to automatic review settings March 4, 2026 05:17
@hotlong hotlong merged commit 19745da into main Mar 4, 2026
4 of 5 checks passed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes the defineStack() validation + second composeStacks() “double-pass” workaround from the dev workspace and console shared config, standardizing on a single @object-ui/core composeStacks() flow so runtime fields (listViews, actions) remain attached to objects.

Changes:

  • Simplified root objectstack.config.ts to a single-pass composeStacks() output passed into AppPlugin (no defineStack()).
  • Simplified apps/console/objectstack.shared.ts to use composed.apps / composed.reports (with the CRM navigation patch applied post-composition) and removed defineStack() + second-pass restoration.
  • Added composeStacks() unit tests for objectConflict: 'error' and prefixed object-name coexistence; updated ROADMAP with the spec/core responsibility split.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
packages/core/src/utils/tests/compose-stacks.test.ts Adds/extends coverage for conflict behavior and prefixed object naming scenarios.
objectstack.config.ts Removes defineStack() and the second-pass restore; uses single composed config for AppPlugin.
apps/console/objectstack.shared.ts Uses composed apps/reports consistently; drops validation + second-pass hack; keeps CRM nav patch.
ROADMAP.md Documents the new single-pass flow and clarifies compose responsibility split (core vs spec).

Comment on lines +285 to +298
it('should detect conflicting object names with objectConflict: "error"', () => {
// Simulates a scenario where two plugins define 'account' without prefixing
const crm = {
objects: [{ name: 'account', label: 'CRM Account', fields: {} }],
};
const ks = {
objects: [{ name: 'account', label: 'KS Account', fields: {} }],
};

expect(() => composeStacks([crm, ks], { objectConflict: 'error' })).toThrow(
'duplicate object name "account"'
);
});

Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conflict test duplicates the existing should throw on duplicate objects when objectConflict is "error" coverage earlier in the file. Consider removing it or adjusting it to cover a distinct case so the suite stays lean and easier to maintain.

Suggested change
it('should detect conflicting object names with objectConflict: "error"', () => {
// Simulates a scenario where two plugins define 'account' without prefixing
const crm = {
objects: [{ name: 'account', label: 'CRM Account', fields: {} }],
};
const ks = {
objects: [{ name: 'account', label: 'KS Account', fields: {} }],
};
expect(() => composeStacks([crm, ks], { objectConflict: 'error' })).toThrow(
'duplicate object name "account"'
);
});

Copilot uses AI. Check for mistakes.
Comment on lines +34 to +42
const dashboardIdx = crmApp.navigation.findIndex((n: any) => n.id === 'nav_dashboard');
const insertIdx = dashboardIdx !== -1 ? dashboardIdx + 1 : 0;
crmApp.navigation.splice(insertIdx, 0, {
id: 'nav_sales_report',
type: 'url',
url: '/apps/crm_app/report/sales_performance_q1',
label: 'Sales Report',
icon: 'file-bar-chart'
});
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation in this block is inconsistent with the surrounding 2-space indentation used in this file. Running the formatter (or adjusting indentation here) will improve readability and reduce noisy diffs in future edits.

Suggested change
const dashboardIdx = crmApp.navigation.findIndex((n: any) => n.id === 'nav_dashboard');
const insertIdx = dashboardIdx !== -1 ? dashboardIdx + 1 : 0;
crmApp.navigation.splice(insertIdx, 0, {
id: 'nav_sales_report',
type: 'url',
url: '/apps/crm_app/report/sales_performance_q1',
label: 'Sales Report',
icon: 'file-bar-chart'
});
const dashboardIdx = crmApp.navigation.findIndex((n: any) => n.id === 'nav_dashboard');
const insertIdx = dashboardIdx !== -1 ? dashboardIdx + 1 : 0;
crmApp.navigation.splice(insertIdx, 0, {
id: 'nav_sales_report',
type: 'url',
url: '/apps/crm_app/report/sales_performance_q1',
label: 'Sales Report',
icon: 'file-bar-chart',
});

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

merge 配置未彻底解决、配置 double-pass hack 和对象命名冲突问题待修复

3 participants