fix(cli): summarize nested user/owner fields in -o table (FT-1942)#39
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (15)
WalkthroughThis PR establishes object value summarisation utilities and extends entity-row formatting across the CLI's list commands. It introduces formatUserSummary and summarizeObjectValue in the formatter module to extract concise display strings from user and object values, then defines four new row-summariser functions in the entity-summary API (summarizeTagRow, summarizeMetricCategoryRow, summarizeNamedEntityRow, summarizeWebhookRow) that curate entity fields and flatten creator/updater information. These summarisers are wired into list commands for API keys, apps, environments, goal tags, metric categories, metric tags, roles, tags, units, and webhooks, with tests updated to verify the richer output shapes. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/api-client/entity-summary.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/api-client/entity-summary.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/commands/apikeys/apikeys.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/commands/apps/apps.test.ts (1)
56-59: ⚡ Quick winAdd coverage for
created_bysummarisation in the list assertion.This matcher now only checks
id/name, so the newcreated_byformatting path can regress unnoticed.Suggested test tightening
- const mockClient = { - listApplications: vi.fn().mockResolvedValue([{ id: 1, name: 'web' }]), + const mockClient = { + listApplications: vi.fn().mockResolvedValue([ + { + id: 1, + name: 'web', + created_by: { first_name: 'Ada', last_name: 'Lovelace', email: 'ada@example.com' }, + }, + ]), @@ - expect(printFormatted).toHaveBeenCalledWith( - [expect.objectContaining({ id: 1, name: 'web' })], - expect.anything() - ); + expect(printFormatted).toHaveBeenCalledWith( + [expect.objectContaining({ id: 1, name: 'web', created_by: 'Ada Lovelace' })], + expect.anything() + );🤖 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 `@src/commands/apps/apps.test.ts` around lines 56 - 59, Update the test assertion in apps.test.ts that checks printFormatted to also assert the created_by summarisation is present and correctly formatted: locate the expect(printFormatted).toHaveBeenCalledWith(...) assertion and extend the expect.objectContaining payload for the app entry (id: 1, name: 'web') to include a created_by property check (use a matcher like expect.stringMatching or expect.any(String) with a regex that matches the expected summary format) so the test will fail if the created_by formatting path regresses.
🤖 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 `@src/commands/apikeys/apikeys.test.ts`:
- Line 115: The test contains a secret-like literal assigned to hashed_key which
can trigger leak scanners; replace the real-looking value in the test (the
hashed_key string in src/commands/apikeys/apikeys.test.ts) with an obviously
synthetic placeholder (e.g., "FAKE_HASHED_KEY_PLACEHOLDER" or similar) so it no
longer resembles a real credential; update any assertions or fixtures that
reference this exact value to use the new placeholder constant to keep tests
consistent.
---
Nitpick comments:
In `@src/commands/apps/apps.test.ts`:
- Around line 56-59: Update the test assertion in apps.test.ts that checks
printFormatted to also assert the created_by summarisation is present and
correctly formatted: locate the expect(printFormatted).toHaveBeenCalledWith(...)
assertion and extend the expect.objectContaining payload for the app entry (id:
1, name: 'web') to include a created_by property check (use a matcher like
expect.stringMatching or expect.any(String) with a regex that matches the
expected summary format) so the test will fail if the created_by formatting path
regresses.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85e72daf-eeca-4159-b453-93891ba52240
📒 Files selected for processing (19)
src/api-client/entity-summary.test.tssrc/api-client/entity-summary.tssrc/commands/apikeys/apikeys.test.tssrc/commands/apikeys/index.tssrc/commands/apps/apps.test.tssrc/commands/apps/index.tssrc/commands/envs/envs.test.tssrc/commands/envs/index.tssrc/commands/goaltags/index.tssrc/commands/metriccategories/index.tssrc/commands/metrictags/index.tssrc/commands/roles/index.tssrc/commands/roles/roles.test.tssrc/commands/tags/index.tssrc/commands/units/index.tssrc/commands/units/units.test.tssrc/commands/webhooks/index.tssrc/lib/output/formatter.test.tssrc/lib/output/formatter.ts
…stringify
Nested objects (created_by, updated_by, owner, etc.) rendered in -o table /
-o vertical / -o markdown / -o plain are now rendered as a short summary
('First Last', email, name, title, or [object]) instead of a JSON blob. -o
json / -o yaml are unchanged. (FT-1942)
…ist -o table (FT-1942)
- replace secret-like hashed_key literal with synthetic placeholder - assert created_by summarisation in apps list test
joalves
force-pushed
the
feat/FT-1942/table-user-summary
branch
from
736a492 to
7eddafb
Compare
1 participant
Summary
-o table/-o vertical/-o markdown/-o plainwith a smart summary:First Last, email,name,title, or the literal[object]. The root cause wasformatValuedoingJSON.stringify(value)on any nested object, which turned everycreated_bycell into a 600-char wall of JSON.summarizeRowto the list commands that previously dumped every API field:apikeys,envs,goaltags,metriccategories,metrictags,roles,tags,units,webhooksappslist summariser to reuse the sharedformatUserSummaryhelper.formatOwnerinentity-summary.tsnow delegates to the sameformatUserSummary, so curated row summaries and the default formatter agree.-o jsonand-o yamlare unchanged (they bypassformatValueentirely).--rawstill returns the full untransformed API response for any list command.JIRA
FT-1942 — CLI: clean up table output for commands that dump user/owner JSON blobs
Before / After
Before (the prompt that motivated this work — apikeys list -o table):
```
│ created_by │
│ {"id":4,"external_id":null,"email":"marcio@absmartly.com","first_name":"Márcio","last_name":"Martins",...} │
```
After:
```
│ created_by │
│ Márcio Martins │
```
Notes for reviewers
formatOwnerand theappssummarizeRowpreviously treated non-null/non-undefinedvalues as user-shaped. They now require an actual object. In practice the API always returns either a User object ornull/missing, so this is safer and not a regression.summarizeApiKeyRow/summarizeTagRowstyle added here, but inconsistent withsummarizeGoal/summarizeUserRowwhich run them throughformatDate). Aligning all summarizers onformatDatecould be a small follow-up.Test plan
Summary by CodeRabbit
New Features
Tests