feat(a2ui): add loading component instead of loading text#2746
Conversation
Sherry-hue
requested review from
HuJean,
PupilTong,
fzx2666-fz and
gaoachao
as code owners
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new A2UI ChangesLoading Component Feature
Streaming image resolution & action validation
Playground hook & small fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/genui/a2ui/styles/catalog/Button.css (1)
35-36: 💤 Low valueConsider increasing the shadow visibility for clearer button elevation
Button.cssuses0 10px 24px var(--a2ui-color-overlay)for the shadow.--a2ui-color-overlayisrgba(0, 0, 0, 0.05)(light) andrgba(248, 248, 248, 0.08)(dark) intheme.css, so the shadow may read quite subtle/flat despite the large spread.0 10px 24px var(--a2ui-color-overlay), 0 1px 0 rgba(255, 255, 255, 0.4) inset;If this is intended for a minimal aesthetic, keep it; otherwise consider bumping the overlay alpha (or reducing the spread) to better match the desired elevation for primary CTAs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/genui/a2ui/styles/catalog/Button.css` around lines 35 - 36, The box-shadow in Button.css (the rule using "0 10px 24px var(--a2ui-color-overlay), 0 1px 0 rgba(255, 255, 255, 0.4) inset") is too subtle given the current --a2ui-color-overlay values; either increase the alpha for --a2ui-color-overlay in theme.css or adjust the Button.css shadow to reduce spread and/or increase opacity so the elevation reads stronger (e.g., target the primary CTA shadow). Update the --a2ui-color-overlay definition or the specific Button.css rule (referencing --a2ui-color-overlay and the box-shadow declaration) to achieve clearer elevation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/genui/a2ui/styles/catalog/Button.css`:
- Around line 35-36: The box-shadow in Button.css (the rule using "0 10px 24px
var(--a2ui-color-overlay), 0 1px 0 rgba(255, 255, 255, 0.4) inset") is too
subtle given the current --a2ui-color-overlay values; either increase the alpha
for --a2ui-color-overlay in theme.css or adjust the Button.css shadow to reduce
spread and/or increase opacity so the elevation reads stronger (e.g., target the
primary CTA shadow). Update the --a2ui-color-overlay definition or the specific
Button.css rule (referencing --a2ui-color-overlay and the box-shadow
declaration) to achieve clearer elevation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60562344-d8b9-401a-8d4b-f90dc3e1a46b
📒 Files selected for processing (12)
packages/genui/a2ui-playground/lynx-src/a2ui/App.tsxpackages/genui/a2ui-playground/src/catalog/a2ui.tspackages/genui/a2ui/package.jsonpackages/genui/a2ui/src/catalog/Loading/index.tsxpackages/genui/a2ui/src/catalog/index.tspackages/genui/a2ui/src/index.tspackages/genui/a2ui/src/react/A2UIRenderer.tsxpackages/genui/a2ui/styles/catalog/Button.csspackages/genui/a2ui/styles/catalog/Loading.csspackages/genui/server/agent/a2ui-catalog.tspackages/genui/server/agent/a2ui-stream-parser.tspackages/genui/server/agent/catalog/Loading/catalog.json
Merging this PR will improve performance by 11.41%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
Sherry-hue
force-pushed
the
feat/a2ui-loading-component
branch
from
b768fc8 to
62dce13
Compare
|
Actionable comments posted: 0 |
Sherry-hue
force-pushed
the
feat/a2ui-loading-component
branch
from
62dce13 to
6d857f8
Compare
|
Actionable comments posted: 0 |
Sherry-hue
force-pushed
the
feat/a2ui-loading-component
branch
2 times, most recently
from
3492b27 to
46bc53a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/genui/server/agent/image-resolver.ts (1)
173-235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the latest image value authoritative within a batch.
If one batch first emits a non-loadable image value and then later emits a real URL for the same
surfaceId/path, this code still keeps the earlier path marked as pending and still appends the earlier async patch afterward. BecausededupeImageDataPatchesonly removes identical{surfaceId, path, value}triples, the stale resolution can overwrite the later real URL and also force an unnecessaryLoading/restore cycle. Please reconcile pending state against the final value seen for eachsurfaceId/path, and dedupe last-wins by path rather than by path+value.Also applies to: 246-304, 410-479
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/genui/server/agent/image-resolver.ts` around lines 173 - 235, The batch can produce a stale async resolution that overwrites a later in-batch value because dedupeImageDataPatches currently de-duplicates by {surfaceId,path,value}; change the flow so pendingImagePathsBySurface and appended pending restores are reconciled against the final resolved value per path and deduping is last-wins by path: collect all promises in dataResolutions as you already do, then after awaiting Promise.all(dataResolutions) build a Map keyed by `${surfaceId}|${path}` keeping the last resolution for each key (overwriting earlier ones), use that map instead of dedupeImageDataPatches (or change dedupeImageDataPatches to implement last-wins by path), and remove any pendingImagePathsBySurface entries (used by replacePendingPathImagesWithLoading) for keys that now have a non-loading final value so pending restores are not emitted for paths that were resolved later; update usages of dedupeImageDataPatches, pendingImagePathsBySurface, and replacePendingPathImagesWithLoading accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/genui/server/agent/image-resolver.ts`:
- Around line 173-235: The batch can produce a stale async resolution that
overwrites a later in-batch value because dedupeImageDataPatches currently
de-duplicates by {surfaceId,path,value}; change the flow so
pendingImagePathsBySurface and appended pending restores are reconciled against
the final resolved value per path and deduping is last-wins by path: collect all
promises in dataResolutions as you already do, then after awaiting
Promise.all(dataResolutions) build a Map keyed by `${surfaceId}|${path}` keeping
the last resolution for each key (overwriting earlier ones), use that map
instead of dedupeImageDataPatches (or change dedupeImageDataPatches to implement
last-wins by path), and remove any pendingImagePathsBySurface entries (used by
replacePendingPathImagesWithLoading) for keys that now have a non-loading final
value so pending restores are not emitted for paths that were resolved later;
update usages of dedupeImageDataPatches, pendingImagePathsBySurface, and
replacePendingPathImagesWithLoading accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d5f1e67-d07a-482d-86fc-b9d0829870e6
📒 Files selected for processing (21)
.github/a2ui-catalog.instructions.mdpackages/genui/a2ui-playground/lynx-src/a2ui/App.tsxpackages/genui/a2ui-playground/src/catalog/a2ui.tspackages/genui/a2ui-playground/src/hooks/useConversation.tspackages/genui/a2ui/etc/genui-a2ui.api.mdpackages/genui/a2ui/package.jsonpackages/genui/a2ui/src/catalog/Loading/index.tsxpackages/genui/a2ui/src/catalog/index.tspackages/genui/a2ui/src/index.tspackages/genui/a2ui/src/react/A2UIRenderer.tsxpackages/genui/a2ui/styles/catalog/Button.csspackages/genui/a2ui/styles/catalog/Loading.csspackages/genui/server/agent/a2ui-catalog.tspackages/genui/server/agent/a2ui-stream-parser.tspackages/genui/server/agent/a2ui-validator.tspackages/genui/server/agent/catalog/Loading/catalog.jsonpackages/genui/server/agent/image-resolver.tspackages/genui/server/app/a2ui/_shared.tspackages/genui/server/app/a2ui/action/route.tspackages/genui/server/app/a2ui/action/stream/route.tspackages/genui/server/app/a2ui/stream/route.ts
💤 Files with no reviewable changes (1)
- packages/genui/server/agent/a2ui-validator.ts
✅ Files skipped from review due to trivial changes (2)
- packages/genui/a2ui-playground/lynx-src/a2ui/App.tsx
- packages/genui/a2ui/etc/genui-a2ui.api.md
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/genui/server/agent/catalog/Loading/catalog.json
- packages/genui/a2ui-playground/src/catalog/a2ui.ts
- packages/genui/a2ui/styles/catalog/Button.css
- packages/genui/a2ui/styles/catalog/Loading.css
- packages/genui/a2ui/src/react/A2UIRenderer.tsx
- packages/genui/a2ui/src/index.ts
- packages/genui/a2ui/package.json
- packages/genui/a2ui/src/catalog/Loading/index.tsx
Sherry-hue
force-pushed
the
feat/a2ui-loading-component
branch
from
46bc53a to
923e1d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/genui/a2ui-playground/src/hooks/useConversation.ts`:
- Around line 83-90: The catch in cloneDataValue currently returns the original
reference on JSON cloning failure which allows shared references; change it to
return a safe, non-referential fallback (consistent with cloneDataModel) instead
of returning value—i.e., when value is an object and JSON.parse/JSON.stringify
throws, return an empty plain object (or an empty array if you detect the
original was an array) so callers of cloneDataValue do not receive the original
reference; update cloneDataValue accordingly and keep primitive handling
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24fc7c1d-3176-4103-862e-c0386f078efc
📒 Files selected for processing (21)
.github/a2ui-catalog.instructions.mdpackages/genui/a2ui-playground/lynx-src/a2ui/App.tsxpackages/genui/a2ui-playground/src/catalog/a2ui.tspackages/genui/a2ui-playground/src/hooks/useConversation.tspackages/genui/a2ui/etc/genui-a2ui.api.mdpackages/genui/a2ui/package.jsonpackages/genui/a2ui/src/catalog/Loading/index.tsxpackages/genui/a2ui/src/catalog/index.tspackages/genui/a2ui/src/index.tspackages/genui/a2ui/src/react/A2UIRenderer.tsxpackages/genui/a2ui/styles/catalog/Button.csspackages/genui/a2ui/styles/catalog/Loading.csspackages/genui/server/agent/a2ui-catalog.tspackages/genui/server/agent/a2ui-stream-parser.tspackages/genui/server/agent/a2ui-validator.tspackages/genui/server/agent/catalog/Loading/catalog.jsonpackages/genui/server/agent/image-resolver.tspackages/genui/server/app/a2ui/_shared.tspackages/genui/server/app/a2ui/action/route.tspackages/genui/server/app/a2ui/action/stream/route.tspackages/genui/server/app/a2ui/stream/route.ts
💤 Files with no reviewable changes (1)
- packages/genui/server/agent/a2ui-validator.ts
✅ Files skipped from review due to trivial changes (4)
- packages/genui/a2ui/package.json
- packages/genui/a2ui/src/index.ts
- packages/genui/a2ui/styles/catalog/Button.css
- packages/genui/a2ui/etc/genui-a2ui.api.md
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/genui/a2ui/src/catalog/index.ts
- packages/genui/server/agent/a2ui-catalog.ts
- .github/a2ui-catalog.instructions.md
- packages/genui/server/agent/catalog/Loading/catalog.json
- packages/genui/a2ui-playground/lynx-src/a2ui/App.tsx
- packages/genui/a2ui-playground/src/catalog/a2ui.ts
- packages/genui/a2ui/src/catalog/Loading/index.tsx
- packages/genui/server/app/a2ui/stream/route.ts
- packages/genui/a2ui/styles/catalog/Loading.css
- packages/genui/server/app/a2ui/_shared.ts
- packages/genui/server/agent/a2ui-stream-parser.ts
- packages/genui/server/app/a2ui/action/route.ts
- packages/genui/server/app/a2ui/action/stream/route.ts
- packages/genui/server/agent/image-resolver.ts
3 participants
Summary by CodeRabbit
Checklist