feat: integrate SIDC Kit feature stack - #22
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR adds curated symbol catalogs and land-unit fixture generation, moves the public SIDC API into dedicated modules, expands browser and CLI packaging, and updates tests and documentation for the new search, explain, render, build, and identify behavior. ChangesCurated SIDC catalog and runtime updates
Sequence Diagram(s)sequenceDiagram
participant run
participant parseArgs
participant searchSymbols
participant explainSidc
participant renderSymbol
participant buildSidc
run->>parseArgs: parse argv
alt search
run->>searchSymbols: query terms and limit
else explain
run->>explainSidc: SIDC
else render
run->>renderSymbol: SIDC and render options
else build
run->>buildSidc: structured parts
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
… codex/integrate-feature-docs
…to codex/integrate-feature-docs # Conflicts: # README.md
…dex/integrate-feature-docs # Conflicts: # README.md # test/sidc-kit.test.mjs
…to codex/integrate-feature-docs # Conflicts: # src/data/symbols.ts # test/sidc-kit.test.mjs
…o codex/integrate-feature-docs # Conflicts: # README.md # package.json # src/data/symbols.ts
…ate-feature-docs # Conflicts: # package-lock.json # package.json
… into codex/integrate-feature-docs # Conflicts: # README.md # src/index.ts # test/sidc-kit.test.mjs
the-Drunken-coder
changed the title
Owner
Author
|
@greptileai review |
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa6960f33b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/index.ts (1)
232-243: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
identifySymbolre-renders the full curated catalog on every call.Each invocation calls
renderSymbol(symbol.sidc, ...)for every curated symbol and then runs an O(L²) Levenshtein over SVG strings up tomaxIdentifySvgInputLength(10k). The length-similarity gate helps, but rendering is repeated work that doesn't depend on the input. Consider memoizing normalized candidate SVGs perrenderOptionsto avoid re-rendering on each call.🤖 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/index.ts` around lines 232 - 243, `identifySymbol` is re-rendering every curated symbol on each call, which repeats expensive work that only depends on `renderOptions`. Cache or memoize the normalized SVG candidate for each `symbol.sidc` within the `identifySymbol` flow (or a helper it uses) so repeated invocations reuse precomputed `renderSymbol(...).svg` output instead of recalculating it. Keep the existing `normalizeSvgInput`, `svgSimilarity`, and threshold filtering logic intact, and make the cache key include the relevant `renderOptions` state.
🤖 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 `@README.md`:
- Around line 138-140: Remove the duplicated Coverage paragraph in README so the
section appears only once; keep the single paragraph about partial
decomposition, entity labels from curated function IDs, and status handling via
unknownFields, and delete the repeated copy of that text.
In `@src/cli.ts`:
- Around line 126-138: The text output path in the CLI currently assumes
explainSidc always returns a name, but partial results can have name undefined
and end up printing undefined. Update the explain command flow in the cli.ts
logic around explainSidc and writeOutput so the name line is only emitted when
result.name is present, and otherwise skip or substitute a clean fallback for
partial non-curated SIDCs. Add or update a CLI test for text mode on a
renderable non-curated SIDC to verify the output does not include undefined.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 232-243: `identifySymbol` is re-rendering every curated symbol on
each call, which repeats expensive work that only depends on `renderOptions`.
Cache or memoize the normalized SVG candidate for each `symbol.sidc` within the
`identifySymbol` flow (or a helper it uses) so repeated invocations reuse
precomputed `renderSymbol(...).svg` output instead of recalculating it. Keep the
existing `normalizeSvgInput`, `svgSimilarity`, and threshold filtering logic
intact, and make the cache key include the relevant `renderOptions` state.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ee7d164-5add-4c9b-abe1-0ac48eb66731
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdfixtures/curated-land-units.jsonpackage.jsonscripts/browser-smoke.mjsscripts/generate-curated-fixtures.mjssrc/cli.tssrc/data/catalog/air.tssrc/data/catalog/control-measures.tssrc/data/catalog/index.tssrc/data/catalog/land-equipment.tssrc/data/catalog/land-installations.tssrc/data/catalog/land-units.tssrc/data/catalog/sea-subsurface.tssrc/data/catalog/sea-surface.tssrc/data/catalog/types.tssrc/data/symbols.tssrc/index.tstest/cli.test.mjstest/sidc-kit.test.mjs
the-Drunken-coder
marked this pull request as ready for review
June 26, 2026 08:41
Owner
Author
|
@codex review |
Owner
Author
|
@greptile review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 408e1f9a6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/search.ts (1)
57-71: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDefer
explainSymbol()until after filtering and limiting.The current search path builds full explain results for every curated symbol, including zero-score entries and matches beyond
limit. Score first, then materialize only returned results.Proposed refactor
return curatedSymbols .map((symbol, index) => { const score = scoreSymbol(symbol, terms, normalizedQuery); return { index, - result: { - ...explainSymbol(symbol), - score - } + score, + symbol }; }) - .filter(({ result }) => result.score > 0) - .sort((left, right) => right.result.score - left.result.score || left.index - right.index) - .map(({ result }) => result) - .slice(0, limit); + .filter(({ score }) => score > 0) + .sort((left, right) => right.score - left.score || left.index - right.index) + .slice(0, limit) + .map(({ score, symbol }) => ({ + ...explainSymbol(symbol), + score + })); }🤖 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/search.ts` around lines 57 - 71, The search pipeline in `search` eagerly calls `explainSymbol()` for every curated symbol before filtering and slicing, which materializes unnecessary results. Update the flow so `scoreSymbol()` is applied first, then filter and sort by score, apply the `limit`, and only then call `explainSymbol()` on the final returned items. Keep the existing ordering logic in `search` intact while moving the expensive materialization step to the end.
🤖 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/build.ts`:
- Around line 38-60: The ambiguity suggestion in buildAmbiguitySuggestion() is
too strong because formatPartList() can imply that any single missing key will
resolve the match. Update the wording so it clearly allows multiple required
discriminators, using the helpfulKeys list from hasVariation() without
suggesting one field is sufficient; keep the fallback message for cases where no
helpful keys exist and adjust the returned message text to be more conditional.
In `@src/identify.ts`:
- Around line 97-108: The SVG data URL decoding in decodeInlineSvgDataUrl
currently assumes every data:image/svg+xml payload is URI-encoded, so
base64-encoded SVGs are never normalized and identifySymbol() misses them.
Update decodeInlineSvgDataUrl to detect ;base64 payloads in addition to plain
encoded SVG data URLs, decode the payload appropriately before returning, and
keep the existing normalization path in normalizeSvgInput() working for both
formats.
---
Nitpick comments:
In `@src/search.ts`:
- Around line 57-71: The search pipeline in `search` eagerly calls
`explainSymbol()` for every curated symbol before filtering and slicing, which
materializes unnecessary results. Update the flow so `scoreSymbol()` is applied
first, then filter and sort by score, apply the `limit`, and only then call
`explainSymbol()` on the final returned items. Keep the existing ordering logic
in `search` intact while moving the expensive materialization step to the end.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc65ca0f-710e-42cb-b3c4-64b55b212087
📒 Files selected for processing (12)
package.jsonsrc/build.tssrc/explain.tssrc/identify.tssrc/index.tssrc/milsymbol.tssrc/render.tssrc/search.tssrc/sidc.tssrc/types.tstest/sidc-kit.test.mjstsconfig.json
💤 Files with no reviewable changes (1)
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (2)
- test/sidc-kit.test.mjs
- package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/data/catalog/land-units.ts, preserving browser smoke + fixture checks in package scripts, and making reverse lookup results curated-onlyIntegrated PRs
Validation
npm testnpm pack --dry-runNotes
This PR supersedes the seven separate feature PRs once reviewers are comfortable with the combined stack. The original PRs are still open and were not closed by this update.
Summary by CodeRabbit
identifySymbolto recognize curated symbols from clean SVG/SVG data-URLs with confidence + evidence.sidc-kitCLI binary, with new commands/options.Greptile Summary
This PR integrates the SIDC Kit feature stack into one package update. The main changes are:
sidc-kitCLI entrypoint.Confidence Score: 5/5
The integrated package behavior is runnable across the API, CLI, fixture, browser-smoke, and packaging paths without identified merge-blocking issues.
None.
What T-Rex did
Comments Outside Diff (2)
General comment
030310001412110000000000000000,node dist/cli.js explain <sidc> --jsonreturns a valid partial decomposition without aname, but the plain-text CLI path prints the missingresult.namedirectly. The first stdout line is the literal textundefined, followed by otherwise useful partial SIDC details.src/cli.tswrites${result.name}unconditionally in the plain-textrunExplainbranch even though partialexplainSidcresults do not include anameproperty.result.name ?? "Partial SIDC decomposition", or branch onresult.coveragebefore rendering the heading.General comment
explainSidcfails on renderable, non-curated variants of curated domain-level symbols. A neutral variant of the curated air fighter SIDC (130401000011010400000000000000) is renderable and preserves function ID1101040000, but before the fix the partial explanation omitted the catalog-backed entity metadata and markedentityunknown despite the curated air catalog containing the same function ID.buildPartialPartslooked upfunctionEntityPartsusinggetSymbolSetLabel(domain, metadata). For domain-level air symbols,getSymbolSetLabelreturnedundefinedunless the metadata was unit/installation/activity, so the lookup key never matched catalog keys such asair:1101040000.getSymbolSetLabelwhen the metadata is not a unit, installation, or activity. This allows partial lookups to use keys likeair:1101040000and resolve catalog-backed entity metadata for domain-level symbols.Reviews (9): Last reviewed commit: "fix: clarify ambiguous build suggestions" | Re-trigger Greptile