Skip to content

feat(ui): optimize hover interactions and user experience#6

Closed
lucky20160622 wants to merge 5 commits into
fim-ai:masterfrom
lucky20160622:master
Closed

feat(ui): optimize hover interactions and user experience#6
lucky20160622 wants to merge 5 commits into
fim-ai:masterfrom
lucky20160622:master

Conversation

@lucky20160622

Copy link
Copy Markdown

What

Improve user experience by adding and optimizing hover interactions and UI feedback.

Why

Enhance usability, visual responsiveness, and overall user experience for better interaction clarity.

Type

  • Bug fix
  • Security patch
  • Documentation
  • New feature (discussed in issue first)
  • Refactoring
  • Test coverage
  • Translation / i18n

Checklist

  • uv run ruff check src/ tests/ passes
  • uv run pytest passes
  • cd frontend && pnpm build passes (if frontend changes)
  • i18n strings added to both en/ and zh/ (if UI text changed)
  • Tests added for new functionality

赵瑞轩 and others added 3 commits April 22, 2026 18:22
…skill card layouts

- Changed connector routing to redirect to the main connectors page after saving a new connector.
- Enhanced layout of agent, connector, knowledge base, and skill cards for better readability and consistency.
- Updated button styles and added edit options for owners in various components.
- Improved translation handling in agent settings form for create/save actions.
@tao-hpu

tao-hpu commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Thanks Ray — the card-level UX rework is a real improvement. Moving primary actions (Run / View / Chat / Edit) out of the hover dropdown into persistent CTAs makes the gallery much more discoverable, and the <h3> → flex wrapper + <span className="truncate"> refactor is the right fix for Tailwind's classic "truncate on a flex container doesn't work" trap. Nice catches.

A few things to tighten up before I can merge:


🔴 Blocker: unintended routing regression

In frontend/src/app/agents/[id]/page.tsx (line 95) and frontend/src/app/connectors/[id]/page.tsx (line 102):

- router.replace(`/agents/${saved.id}`)
+ router.replace("/agents")

This changes post-create behavior from "land on the new thing you just made" to "bounce back to the list". That's a regression — users lose editing context on the resource they just created, and it contradicts standard SaaS patterns (Notion / Linear / Supabase all keep you on the newly-created entity). It also isn't mentioned in the PR description and seems out of scope for "hover interactions". Please revert both lines.


🔴 Blocker: i18n workflow violation

You added an enter key to all 6 locales (messages/{en,zh,ja,ko,de,fr}/kb.json), but per CLAUDE.md:

Only edit: messages/en/{ns}.json. All non-English files are auto-generated by scripts/translate.py on pre-commit.

Please:

  1. Revert the zh/ja/ko/de/fr changes — leave only messages/en/kb.json.
  2. Make sure your .env has a working LLM key (OPENAI_API_KEY or configured translator endpoint in example.env) so the pre-commit hook can actually translate. Without the key the hook silently skips and the translations won't land.
  3. Bootstrap hooks once if you haven't: bash scripts/setup-hooks.sh.

Also — the enter key doesn't appear to be referenced anywhere in the diff. Either wire it into the component that needs it, or remove it entirely. Dead i18n keys quietly pile up.


🟡 Consistency nit: Connector card only shows CTA for owners

Other cards (Agent / KB / Workflow) expose a primary CTA to everyone (View / Run / Start Chat) and add Edit for owners. connector-card.tsx only renders the CTA region when isOwner, so non-owners see a card with no action affordance at all. Consider mirroring the pattern — maybe a "View" / "Test" CTA for non-owners.


🟡 Please complete the checklist

  • cd frontend && pnpm build — we require this before push for frontend changes, catches SSR / type / HMR-unsafe issues the dev server misses.
  • Manual smoke: create a new Agent → confirm you land on its edit page (after the revert above), then save → confirm "Create" label flips to "Save".

Once the two blockers are addressed and build passes, happy to approve and merge. Appreciate the patience — the UX direction is solid.

@lucky20160622

Copy link
Copy Markdown
Author

Thanks for the careful review and for calling out the UX improvements.

On the product behavior changes, I’d like to clarify that they were intentional rather than accidental side effects.

For the post-create redirect behavior, my reasoning was that these resources in FIM One currently behave more like configurable assets than long-form editors. In that context, returning users to the collection view after first creation can help them continue managing the set, instead of forcing them to stay inside a detail page they may not need immediately. So from my perspective, this is a product choice rather than a routing regression.

Similarly, the card CTA changes were designed around discoverability and action clarity across the gallery. Persistent primary actions reduce hidden affordances and make the system easier to scan, especially for first-time users.

That said, I understand your preference for aligning with the more common SaaS convention of staying on the newly created entity. If consistency with the existing product pattern is more important here, I’m happy to defer to that direction — but I wanted to explain that these changes were deliberate UX decisions, not accidental scope creep.

@tao-hpu

tao-hpu commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Appreciate the thoughtful pushback, Ray — and thanks for landing the i18n cleanup (3820e2a). That part's clean now.

On the post-create redirect, I want to engage with your "configurable assets vs long-form editors" framing because it's a fair lens, but I don't think it lands for Agent and Connector specifically in FIM One's current shape:

Why Agent/Connector are heavy-configuration, not light assets

  • Connector creation is a multi-step flow by design. Look at connector-settings-form.tsx — creating the connector is just step 1. Step 2 is adding actions, step 3 is per-action JMESPath / schema / auth / confirmation wiring. Every one of those steps happens on the detail page. Bouncing to /connectors right after save means the user's next action is always to click back into the thing they just created. That's friction, not flow.
  • Agents follow the same pattern. A freshly-created agent has no system prompt tuned, no tools attached, no KB bound, no model preferences set. The canonical next step is to stay on the edit page and configure those. Sending the user to /agents right after save optimizes for the (rarer) case of "done, next" at the cost of the (common) case of "now let me actually set this up".
  • router.replace vs router.push — worth flagging: the current change uses replace, which means the back button won't return them to the page they just created either. If we ever did want the list-first flow, push would at least preserve navigation history. That's a separate detail but suggests the change wasn't fully pressure-tested.

Why this belongs in its own PR regardless

Even if the list-first redirect turned out to be the better UX — which I don't think it is, but reasonable people can disagree — changing post-create routing is a product decision, not a UI affordance one. It deserves its own issue for discussion, its own PR, and ideally some signal (usage data, user feedback) to weigh against the current convention. Bundling it into a "hover interactions and UX" PR means reviewers and future git-blame readers won't find the reasoning where they expect it.

Concretely, for this PR I'd like to:

  1. Revert those two router.replace lines — keep the scope tight around the card/CTA work, which is the actual win here.
  2. If you feel strongly about the list-first flow, please open a separate issue describing the rationale. Happy to discuss it there on its own merits, and if the team aligns on the change it can land cleanly with its own reasoning documented.

Still open from the previous round:

  • 🟡 Connector card non-owner CTA asymmetry — mentioned in my first review, not addressed yet. Non-owners currently see no primary action on a connector card while they do on Agent/KB/Workflow. Either a "View" button for non-owners, or at minimum a comment explaining the asymmetry is intentional.
  • pnpm build passing — please confirm this ran locally and passed. Frontend changes this structural (flex reshuffle on 5 card components) sometimes trip SSR / RSC boundaries the dev server doesn't surface.

Once the two router.replace lines are reverted and build confirmed, I'm ready to approve. Really do appreciate that you took the time to lay out your reasoning — that's how this gets to the right answer.

@tao-hpu

tao-hpu commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Short version: once the items below are green, I'll merge. Laying it out as one list so you know exactly what "done" looks like — no more one-at-a-time ping-pong after this round.


Must-fix before merge

  1. Revert the two router.replace lines in frontend/src/app/agents/[id]/page.tsx and frontend/src/app/connectors/[id]/page.tsx — back to router.replace(\/agents/${saved.id}`)/router.replace(`/connectors/${saved.id}`)`. If you feel strongly about the list-first flow, I'd genuinely welcome a dedicated issue — happy to discuss it on its own merits. Just not bundled into this PR.

  2. Confirm cd frontend && pnpm build passes locally — the flex reshuffle across 5 card components is structural enough that dev-server-only testing isn't sufficient. A quick "build passes on my machine" in the comments is enough.

  3. Connector card CTA for non-owners — currently connector-card.tsx wraps the new CTA row in {isOwner && ...}, so non-owners see a card with no action affordance at all, while Agent / KB / Workflow cards show a primary CTA to everyone. Either mirror the pattern (a "View" or "Test" CTA for non-owners), or leave a brief code comment explaining the asymmetry is intentional. Either is fine — just want it to be a deliberate decision.

Quick verify (should be one-line)

  1. In 3820e2a the commit message says "remove unused kb enter key" — please double-check the enter key is removed from frontend/messages/en/kb.json as well, not just the 5 non-English files. If it's still in EN, the pre-commit translate hook will silently sync it back to all locales on the next run and we'll be back where we started.

Heads-up on post-merge polish

Once this lands, I'll likely do a small pass to tighten cross-card consistency in a follow-up commit — things like unifying mt-auto vs mt-3 on the CTA container so cards line up vertically in grid views, and making sure no card type ends up with a near-empty "…" dropdown after the action migration. None of that is a reflection on your PR — it's cross-cutting polish that's easier to do once all 5 cards share the new structure in master. Flagging it so you're not surprised to see those changes show up on top of your work.

Appreciate the patience on this one — the UX direction you pushed is real value, just want to land it cleanly.

tao-hpu added a commit that referenced this pull request Apr 23, 2026
Code + design contributions via Gitee (no GitHub account).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tao-hpu tao-hpu closed this Apr 23, 2026
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.

2 participants