feat(sfa): Add bindings for archive metadata fetching APIs.#141
feat(sfa): Add bindings for archive metadata fetching APIs.#141Bill-hbrhbr wants to merge 2 commits intoy-scope:mainfrom
Conversation
WalkthroughThis pull request adds new file metadata APIs to the CLP archive reader, exposing Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript<br/>(ClpArchiveReader)
participant WASM as WASM Module<br/>(ClpSfaReader)
participant CPP as C++ Runtime<br/>(SfaReader)
rect rgba(100, 150, 200, 0.5)
Note over JS,CPP: getFileNames() Flow
JS->>WASM: getFileNames()
WASM->>CPP: get_file_names()
CPP->>CPP: build JS array from<br/>m_reader.get_file_names()
CPP-->>WASM: StringArrayTsType
WASM-->>JS: string[]
end
rect rgba(150, 100, 200, 0.5)
Note over JS,CPP: getFileInfos() Flow
JS->>WASM: getFileInfos()
WASM->>CPP: get_file_infos()
CPP->>CPP: build JS array with<br/>{ fileName, logEventIdx*, count }
CPP-->>WASM: FileInfoArrayTsType
WASM-->>JS: FileInfoArray
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/clp_ffi_js/sfa/ClpArchiveReader.ts`:
- Around line 64-66: The return value of getFileInfos() in ClpArchiveReader.ts
is being force-cast to FileInfoArray, masking a WASM binding/type registration
problem; instead, declare FileInfoArrayTsType in the shared binding types header
and register it centrally so the generated WASM binding for
SfaReader::getFileInfos() carries the correct TypeScript type—add
FileInfoArrayTsType to binding_types.hpp and register it in binding_types.cpp
(mirroring StringArrayTsType), regenerate bindings, then remove the "as
FileInfoArray" cast from getFileInfos() so getFileInfos() naturally returns the
proper FileInfoArray type.
In `@taskfiles/deps.yaml`:
- Around line 137-139: The URL for the CLP dependency (the URL key near
FILE_SHA256 and OUTPUT_DIR) points to a non-existent fork/commit and must be
replaced with a valid source; update the URL value to a reachable tarball for a
real commit in the official y-scope/clp repository (or another accessible host)
and then recompute and replace FILE_SHA256 to match the new tarball, ensuring
OUTPUT_DIR remains correct; verify the new URL downloads successfully and the
checksum matches before merging.
In `@test/ClpArchiveReader.test.ts`:
- Around line 55-65: Update the test to assert the shape and consistency of file
metadata beyond counts: after creating the reader via ClpArchiveReader.create
and obtaining fileNames = reader.getFileNames() and fileInfos =
reader.getFileInfos(), add assertions that for each index i
fileInfos[i].fileName === fileNames[i], that expected bigint fields on fileInfos
(e.g., size/offset/eventCount if present) exist and typeof fileInfos[i].<field>
=== "bigint", and that aggregated values derived from fileInfos (sum of per-file
eventCount) equal reader.getEventCount() and the
CLP_JSON_TEST_LOG_FILES_EXPECTED_EVENT_COUNT constant; keep a TODO comment
referencing creating a GitHub issue to ensure clp-ffi-js works from plain JS (no
.d.ts reliance).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 561d4ec9-8435-45bb-8322-56cd49f6ad78
📒 Files selected for processing (12)
CMakeLists.txtsrc/clp_ffi_js/binding_types.cppsrc/clp_ffi_js/binding_types.hppsrc/clp_ffi_js/sfa/ClpArchiveReader.tssrc/clp_ffi_js/sfa/SfaReader.cppsrc/clp_ffi_js/sfa/SfaReader.hppsrc/clp_ffi_js/sfa/index.tssrc/clp_ffi_js/sfa/types.tstaskfiles/deps.yamltest/ClpArchiveReader.test.tstest/constants.tstest/utils.ts
| getFileInfos (): FileInfoArray { | ||
| return this.#getWasmReader().getFileInfos() as FileInfoArray; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how ClpSfaReader.getFileInfos/getFileNames are typed in TS declarations.
# Expected: getFileInfos() is explicitly typed as FileInfoArray-compatible (not any/unknown).
rg -n --type=ts -C3 '\b(interface|class)\s+ClpSfaReader\b|\bgetFileInfos\s*\(|\bgetFileNames\s*\('Repository: y-scope/clp-ffi-js
Length of output: 2014
🏁 Script executed:
#!/bin/bash
# Find TypeScript declaration files for ClpSfaReader
find . -name "*.d.ts" -type f | head -20Repository: y-scope/clp-ffi-js
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Look for the WASM reader type and getFileInfos declaration
rg -n "getFileInfos\s*\(" --type=ts -B5 -A5 | head -50Repository: y-scope/clp-ffi-js
Length of output: 1814
🏁 Script executed:
#!/bin/bash
# Find where ClpSfaReader is declared or imported
rg -n "ClpSfaReader" --type=ts --type=js | head -30Repository: y-scope/clp-ffi-js
Length of output: 500
🏁 Script executed:
#!/bin/bash
# Check the `#getWasmReader`() method to see what type it returns
rg -n "#getWasmReader\s*\(" --type=ts -A10Repository: y-scope/clp-ffi-js
Length of output: 3020
🏁 Script executed:
#!/bin/bash
# Look for .d.ts files or index files in node_modules or dist that define ClpSfaReader
find . -path ./node_modules -prune -o -name "*.d.ts" -type f -print | xargs grep -l "ClpSfaReader" 2>/dev/nullRepository: y-scope/clp-ffi-js
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check if there's a `@clp-ffi-js/node` or clp-ffi-js/node directory
find . -path ./node_modules -prune -o -type d -name "node" -print | head -10Repository: y-scope/clp-ffi-js
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for type definitions for getFileInfos in the codebase
rg -n "getFileInfos\s*\(\s*\)\s*:" --type=ts --type=tsx | head -20Repository: y-scope/clp-ffi-js
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Look at the FileInfoArray type definition
rg -n "type FileInfoArray|interface FileInfoArray" --type=ts --type=tsx -A5Repository: y-scope/clp-ffi-js
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Find tsconfig.json to understand the path alias
find . -name "tsconfig.json" -o -name "tsconfig.*.json" | head -5Repository: y-scope/clp-ffi-js
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Look for FileInfoArray definition without tsx file type
rg -n "FileInfoArray" --type=ts -B2 -A2 | head -40Repository: y-scope/clp-ffi-js
Length of output: 1465
🏁 Script executed:
#!/bin/bash
# Check package.json for path mappings
cat package.json | head -50Repository: y-scope/clp-ffi-js
Length of output: 1604
🏁 Script executed:
#!/bin/bash
# Look for the actual source of ClpSfaReader by checking imports
rg -n "from.*clp-ffi-js" --type=ts | head -20Repository: y-scope/clp-ffi-js
Length of output: 535
🏁 Script executed:
#!/bin/bash
# Check if dist directory exists and what's in it
ls -la dist/ 2>/dev/null | head -20Repository: y-scope/clp-ffi-js
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Look for the .d.ts file with type definitions
find . -name "ClpFfiJs-node.d.ts" -o -name "*.d.ts" | grep -E "(ClpFfiJs|node)" | head -10Repository: y-scope/clp-ffi-js
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check tsconfig to understand the path alias resolution
cat tsconfig.json | grep -A10 '"paths"'Repository: y-scope/clp-ffi-js
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for ClpSfaReader type definition in source
rg -n "export.*class ClpSfaReader|export.*interface ClpSfaReader" --type=tsRepository: y-scope/clp-ffi-js
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Look at module.ts to understand how the WASM module is structured
cat src/clp_ffi_js/sfa/module.tsRepository: y-scope/clp-ffi-js
Length of output: 862
🏁 Script executed:
#!/bin/bash
# Check for WASM binding or interface definitions
rg -n "interface MainModule|getFileInfos|getFileNames" --type=ts -B3 -A3 | head -60Repository: y-scope/clp-ffi-js
Length of output: 2014
🏁 Script executed:
#!/bin/bash
# Look at the actual SfaReader.cpp to understand WASM bindings
cat src/clp_ffi_js/sfa/SfaReader.cpp 2>/dev/null | head -80Repository: y-scope/clp-ffi-js
Length of output: 3041
🏁 Script executed:
#!/bin/bash
# Find and read binding_types.hpp
find . -name "binding_types.hpp" -o -name "binding_types.*"Repository: y-scope/clp-ffi-js
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Look for type definitions related to TypeScript types
rg -n "StringArrayTsType|FileInfoArrayTsType" --type=hpp --type=h -B2 -A2Repository: y-scope/clp-ffi-js
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Read binding_types.hpp directly
cat src/clp_ffi_js/binding_types.hppRepository: y-scope/clp-ffi-js
Length of output: 406
🏁 Script executed:
#!/bin/bash
# Also check binding_types.cpp
cat src/clp_ffi_js/binding_types.cppRepository: y-scope/clp-ffi-js
Length of output: 416
🏁 Script executed:
#!/bin/bash
# Verify if FileInfoArrayTsType is missing from binding_types.hpp and if adding it would be the proper fix
# Check what imports SfaReader.cpp to see the full picture
grep -n "FileInfoArrayTsType" src/clp_ffi_js/sfa/SfaReader.cppRepository: y-scope/clp-ffi-js
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Check if FileInfoArrayTsType is declared anywhere else
rg -n "FileInfoArrayTsType" --type=cRepository: y-scope/clp-ffi-js
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Look for where FileInfoArrayTsType should be declared
grep -r "EMSCRIPTEN_DECLARE_VAL_TYPE" src/Repository: y-scope/clp-ffi-js
Length of output: 894
Avoid masking type drift with as FileInfoArray.
The cast at line 65 hides a type binding issue. FileInfoArrayTsType is declared in SfaReader.hpp but registered only locally in the WASM bindings, unlike StringArrayTsType which is centrally registered in binding_types.cpp. This prevents TypeScript from recognizing the proper return type of getFileInfos(). Prefer typing FileInfoArrayTsType consistently with StringArrayTsType by declaring it in binding_types.hpp, registering it in binding_types.cpp, and then removing the cast.
Proposed wrapper simplification once WASM binding is corrected
getFileInfos (): FileInfoArray {
- return this.#getWasmReader().getFileInfos() as FileInfoArray;
+ return this.#getWasmReader().getFileInfos();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/clp_ffi_js/sfa/ClpArchiveReader.ts` around lines 64 - 66, The return
value of getFileInfos() in ClpArchiveReader.ts is being force-cast to
FileInfoArray, masking a WASM binding/type registration problem; instead,
declare FileInfoArrayTsType in the shared binding types header and register it
centrally so the generated WASM binding for SfaReader::getFileInfos() carries
the correct TypeScript type—add FileInfoArrayTsType to binding_types.hpp and
register it in binding_types.cpp (mirroring StringArrayTsType), regenerate
bindings, then remove the "as FileInfoArray" cast from getFileInfos() so
getFileInfos() naturally returns the proper FileInfoArray type.
| FILE_SHA256: "37183079c8a42413fa44beba8dcf5e0a47acfe3f55504171c79c35dbf4e4a9d3" | ||
| OUTPUT_DIR: "{{.OUTPUT_DIR}}" | ||
| URL: "https://github.com/y-scope/clp/archive/5798e0e.tar.gz" | ||
| URL: "https://github.com/Bill-hbrhbr/clp/archive/29b0aa6.tar.gz" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the commit exists in the official y-scope/clp repository
echo "Checking if commit 29b0aa6 exists in y-scope/clp..."
gh api repos/y-scope/clp/commits/29b0aa6 --jq '.sha' 2>/dev/null || echo "Commit not found in official repo"
# Check for open PRs from Bill-hbrhbr to y-scope/clp
echo ""
echo "Checking for open PRs from Bill-hbrhbr to y-scope/clp..."
gh pr list --repo y-scope/clp --author Bill-hbrhbr --state open --json number,title,urlRepository: y-scope/clp-ffi-js
Length of output: 516
🏁 Script executed:
# Examine the taskfiles/deps.yaml file and surrounding context
cat -n taskfiles/deps.yaml | sed -n '130,150p'Repository: y-scope/clp-ffi-js
Length of output: 855
🌐 Web query:
y-scope/clp repository commit 29b0aa6
💡 Result:
No public information found on commit 29b0aa6 in the y-scope/clp GitHub repository. Searches across GitHub pages, issues, releases, and general web did not surface any details about this specific commit hash. It may be a private commit, a typo in the hash, or not yet indexed. The repository is YScope's Compressed Log Processor (CLP), an open-source tool for compressing and searching logs without decompression, with recent activity as of March 2026 (last push 2026-03-14, latest release v0.10.0 on 2026-03-09). Repository URL: https://github.com/y-scope/clp
Citations:
- 1: https://github.com/y-scope/clp
- 2: feat(clang-tidy): Fix all warnings and errors for all C++ source files. clp#764
- 3: https://github.com/y-scope/clp/releases
- 4: https://github.com/y-scope
🌐 Web query:
Bill-hbrhbr/clp GitHub fork
💡 Result:
No GitHub repository exists at github.com/Bill-hbrhbr/clp. Bill-hbrhbr (likely Bingran Hu) is a contributor to the y-scope/clp project (Compressed Log Processor), having closed issues like #1352 and authored PRs such as #1994 (bumping go-task version) and #121 (taskfile changes). The user profile at github.com/Bill-hbrhbr mentions CLP but lists no repositories, suggesting any clp-related work is directly in the upstream y-scope/clp repo rather than a personal fork under Bill-hbrhbr/clp. No evidence of a fork named clp by this user was found across searches of forks, repos, or direct URLs.
Citations:
- 1: https://github.com/Bill-hbrhbr
- 2: Unlock task version once upstream issues go-task/task#2401 and go-task/task#2440 are resolved clp#1352
- 3: https://github.com/y-scope/clp
- 4: https://github.com/y-scope
🏁 Script executed:
# Check git log for changes to this file
git log --oneline -10 taskfiles/deps.yamlRepository: y-scope/clp-ffi-js
Length of output: 94
🏁 Script executed:
# Search for any other references to Bill-hbrhbr or this commit hash in the codebase
rg -i "Bill-hbrhbr|29b0aa6" --type yaml --type json --type txtRepository: y-scope/clp-ffi-js
Length of output: 156
Dependency URL references non-existent repository and must be corrected before merge.
The CLP dependency is configured to download from https://github.com/Bill-hbrhbr/clp/archive/29b0aa6.tar.gz, but this repository does not exist. The user Bill-hbrhbr has no public fork of CLP; the commit 29b0aa6 is not present in the official y-scope/clp repository either. This will cause the build to fail at runtime when attempting to download the dependency.
Update the URL to point to a valid commit in the official y-scope/clp repository or another accessible source before this change is merged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@taskfiles/deps.yaml` around lines 137 - 139, The URL for the CLP dependency
(the URL key near FILE_SHA256 and OUTPUT_DIR) points to a non-existent
fork/commit and must be replaced with a valid source; update the URL value to a
reachable tarball for a real commit in the official y-scope/clp repository (or
another accessible host) and then recompute and replace FILE_SHA256 to match the
new tarball, ensuring OUTPUT_DIR remains correct; verify the new URL downloads
successfully and the checksum matches before merging.
| it("should read clp_json_test_log_files sfa archive from buffer", async () => { | ||
| const data = await loadTestData("clp_json_test_log_files.clp"); | ||
| reader = await ClpArchiveReader.create(data); | ||
|
|
||
| const fileNames = reader.getFileNames(); | ||
| expect(fileNames.length).toBe(CLP_JSON_TEST_LOG_FILES_EXPECTED_FILE_COUNT); | ||
|
|
||
| const fileInfos = reader.getFileInfos(); | ||
| expect(fileInfos.length).toBe(CLP_JSON_TEST_LOG_FILES_EXPECTED_FILE_COUNT); | ||
|
|
||
| expect(reader.getEventCount()).toBe(CLP_JSON_TEST_LOG_FILES_EXPECTED_EVENT_COUNT); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen file metadata assertions beyond count checks.
Please also validate fileInfos content (e.g., fileName alignment with getFileNames(), bigint fields present, and aggregate consistency) so API-shape regressions are caught at runtime, not only by TS typing.
Based on learnings: Ensure that clp-ffi-js supports JavaScript usage without relying on the TypeScript definition file, and keep track of related TODOs in a GitHub issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ClpArchiveReader.test.ts` around lines 55 - 65, Update the test to
assert the shape and consistency of file metadata beyond counts: after creating
the reader via ClpArchiveReader.create and obtaining fileNames =
reader.getFileNames() and fileInfos = reader.getFileInfos(), add assertions that
for each index i fileInfos[i].fileName === fileNames[i], that expected bigint
fields on fileInfos (e.g., size/offset/eventCount if present) exist and typeof
fileInfos[i].<field> === "bigint", and that aggregated values derived from
fileInfos (sum of per-file eventCount) equal reader.getEventCount() and the
CLP_JSON_TEST_LOG_FILES_EXPECTED_EVENT_COUNT constant; keep a TODO comment
referencing creating a GitHub issue to ensure clp-ffi-js works from plain JS (no
.d.ts reliance).
Bill-hbrhbr
changed the title
1 participant
Description
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Release Notes
New Features
getFileNames()andgetFileInfos()methods to the archive reader for accessing file metadata and informationFileInfo,FileInfoArray,FieldValue) for better type support in applicationsTests