feat(router): paginate Summary Mode instead of hard-truncating at 5 - #56
feat(router): paginate Summary Mode instead of hard-truncating at 5#56ShutovKS wants to merge 4 commits into
Conversation
Replace hard-coded 5-item truncation with flexible offset/limit pagination.
Breaking changes:
- Arrays >10 items still trigger Summary Mode by default (backward compatible)
- New optional parameters: 'offset' (default: 0), 'limit' (default: 5)
New response format:
{
"isTruncated": true,
"totalCount": N,
"offset": M,
"limit": K,
"showing": X,
"items": [...],
"nextOffset": (M+K if more pages exist),
"hint": "Showing items M-(M+X-1) of N. To see more, pass 'offset=...'"
}
Use cases:
- Agent paginated discovery: query first 20 GameObjects, process batch, fetch next 20
- Progressive loading: offset=0&limit=50, then offset=50&limit=50
- Bypass truncation: verbose=true still returns full array (unchanged)
Validation:
- offset must be non-negative integer (returns TypeMismatch error if invalid)
- limit must be positive integer (returns TypeMismatch error if invalid)
- offset beyond array bounds returns empty page with hint
Parameters added to reserved list to prevent skill name conflicts.
…aging Skills like asset_find and light_find_all declare their own 'limit' parameter. Reading it as the envelope page size also forced the paginated wrapper onto small results, changing their shape. Skip envelope paging for a name the skill declares itself, and factor the argument parsing and pre-invoke unwind into helpers.
|
Okay, I'll review your PR when I have time. |
|
Tested in Unity 6.3 LTS. The documented asset_find pagination example does not work: asset_find returns an object containing assets, while the router only paginates a top-level JArray. offset 0 and offset 2 therefore return the same first items. A structured scan of the current registered skill implementations also found no direct top-level-array endpoint, so the feature is effectively unreachable on the current public surface. Numeric offset: 1.5 is also accepted because ToObject() coerces it. Please target a real response shape or support nested result arrays, enforce true integer validation, guard pagination arithmetic, and add end-to-end tests. |
Besty0728
left a comment
Besty0728
left a comment
There was a problem hiding this comment.
I re-tested the current head (7419ba6) in a real Unity 6.3 LTS 6000.3.9f1 project through the UnitySkills server on port 8090, with Addressables 3.1.0 installed. This revision contains real improvements, but it is still not ready to merge because the following defects are reproducible blockers.
- Summary pagination is still incorrect for skills that already declare limit.
Request:
{"searchFilter":"","offset":2,"limit":2,"verbose":false}
Actual result metadata:
{"totalCount":2,"offset":2,"limit":5,"showing":0,"items":[]}
asset_find consumes limit first and truncates the source array to two elements. The router then applies offset 2 to that already-truncated array and falls back to its default page size of 5. Therefore the documented offset + limit contract still does not work for asset_find. The pagination wrapper also replaces the original result object and loses sibling metadata such as count and totalFound. This needs an unambiguous envelope/skill parameter design, preservation of result metadata, and an end-to-end regression test using the real asset_find endpoint.
- Addressables group creation produces groups that cannot build content.
A group created through addressables_group_create has:
m_SchemaSet:
m_Schemas: []
I added a real asset with a custom address and then called addressables_build. All three endpoints reported success, but the address was absent from the generated catalog and build output, and no bundle contained the asset. The group needs valid BundledAssetGroupSchema and ContentUpdateGroupSchema instances, or schemas copied from an appropriate template/default group.
- addressables_build reports false success.
The reflection call stores the build result in parameters[0], but the implementation never inspects that result. It returns success whenever reflection does not throw. Build result errors must be read and surfaced. Tests must verify the generated catalog/bundle, not only the HTTP success envelope.
- Addressables package version is wrong.
With com.unity.addressables 3.1.0 installed, addressables_check_installed returns version 0.0.0.0 because it reads the assembly version. Please obtain the UPM package version through UnityEditor.PackageManager.PackageInfo.
- Unity 2022 Russian font support does not meet the requested compatibility requirement.
The current implementation only applies the bundled font for Chinese. Russian explicitly clears the custom font and falls back to the Editor default font, and the new test asserts that fallback. The package contains only UnitySkillsCN-Regular.ttf and UnitySkillsCN-UI.asset; there is no pre-baked Russian FontAsset.
Russian text renders without tofu in the tested Unity 6 environment, but that does not establish Unity 2022 compatibility. Unity 2022 requires a persistent, static, pre-baked FontAsset containing every fixed Russian UI character, with an EditMode glyph-coverage test running on the 2022 code path.
- The PR scope no longer matches its title or description.
The PR is titled as a Summary Mode pagination change, but currently changes 31 files with roughly +2830/-378 lines and includes Addressables integration, batch error behavior, localization, fonts, settings, and UI changes. Please split unrelated features or update the PR description and provide a clear verification matrix for each supported Unity version.
- git diff --check still fails because PullRequestRegressionTests.cs.meta lines 9-11 contain trailing whitespace.
Confirmed improvements:
- Root-level batch failure propagation is fixed.
- Fractional offset values now return TYPE_MISMATCH.
- The previous Addressables reflection issues around IsDefaultGroup, ICollection entries, overload selection, and default-group deletion protection are improved.
- PullRequestRegressionTests passes 5/5 with Addressables installed.
- The dynamic two-language design works as requested: Settings can pin English/Russian or Chinese/Russian, while the footer remains exactly two buttons and changes to EN/RU or 中文/RU. Russian text is visually usable in Unity 6.
The passing regression tests are currently too shallow to detect the pagination contract failure, schema-less Addressables groups, false-positive builds, incorrect package version, or the missing Unity 2022 pre-baked Russian font. These blockers need to be addressed before approval.
7419ba6 to
f09a29a
Compare
|
The branch has been cleaned back to pagination-only scope (head
Verification on Unity 6000.3.11f1: focused E2E |
|
Follow-up: after the initial cleanup, the continuation hint was corrected to use |
Summary
Replaces hard truncation in Summary Mode with unambiguous envelope pagination while preserving skill-owned paging parameters and result metadata.
Contract
Envelope pagination uses
pageOffsetandpageLimit:{ "searchFilter": "t:Material", "limit": 100, "pageOffset": 20, "pageLimit": 10, "verbose": false }limitabove still belongs toasset_find;pageLimitbelongs to the router envelope.Behavior
items,assets,objects,groups, orentries.countandtotalFound.offset,limit,showing,nextOffset, and apageOffsetcontinuation hint.TYPE_MISMATCH.offsetandlimitparameters remain untouched.Verification
asset_findE2E: passed.219/219passed.git diff --check: passed.SkillRouter.csand its end-to-end test.