Repository navigation
docs: name the domain in one glossary and fix the viewport claim - #38
Conversation
The repo had no single answer for what a reference, an entry, an asset, and a source each are, so the docs used "screenshot" for all four and the code carried a fourth vocabulary of its own. Add CONTEXT.md as the one glossary, linked from the README and the agent guide. Rename the internal `vrefDir` to `manifestDir`: it is the directory holding the manifest, which is only `.vref/` until `--manifest` moves it. Correct `viewport` everywhere it was described. The field records the CSS viewport a reference represents, while the derived value measures the stored pixels, so they agree only at a device pixel ratio of 1 — not just on retina captures, as the docs and `vref describe` both claimed.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Some remaining user-facing help/schema text still uses the old “retina-only/source-pixels” viewport framing, and the README currently overstates derived-field behavior without noting overrides.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR standardizes domain vocabulary across the project (via a single glossary), corrects documentation around viewport semantics (CSS viewport vs stored pixels), and renames the internal vrefDir path concept to the more accurate manifestDir to reflect non-default manifest locations.
Changes:
- Add
CONTEXT.mdas the canonical glossary and link it fromREADME.mdandAGENTS.md. - Clarify
viewportdocumentation to distinguish CSS viewport from stored pixel dimensions (and when to supply it in--json). - Rename internal path field
vrefDir→manifestDiracross implementation and smoke script.
File summaries
| File | Description |
|---|---|
| src/screenshot-add.ts | Switch asset path operations from paths.vrefDir to paths.manifestDir. |
| src/path-safety.ts | Rename WorkspacePaths.vrefDir to manifestDir and derive it from manifestPath. |
| src/manifest-edit.ts | Update manifest edit logic to use paths.manifestDir for asset resolution/existence checks. |
| src/describe.ts | Update vref describe notes to clarify viewport intent vs defaulting behavior. |
| src/convert.ts | Update conversion pipeline to use manifestDir for reads/writes and collision checks. |
| src/build.ts | Update validation path safety checks to use manifestDir. |
| skills/vref/SKILL.md | Update agent guidance for when to set viewport (DPR != 1) and why. |
| skills/vref/references/manifest.md | Refine rules around viewport to reflect CSS viewport vs stored pixels. |
| scripts/smoke.ts | Rename local .vref directory variable for clarity (manifestDirectory). |
| README.md | Refine derived-field wording and link to the new glossary. |
| docs/VREF.md | Correct viewport explanation and when to provide it in --json. |
| CONTEXT.md | Add a canonical glossary for reference/entry/asset/source and related terminology. |
| AGENTS.md | Link the glossary in the “Start Here” section. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 0a893d8b8a
ℹ️ 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".
Keying the viewport rule to device pixel ratio was browser-only framing for a tool whose own running example is Roku, where there is no CSS viewport and a scaled or cropped capture differs from its stored pixels for reasons DPR does not describe. Say logical dimensions, and to override whenever the two differ. Two surfaces still carried the old framing: the `screenshot add` help text and the `screenshotDraft.derivedFields` entry in `vref describe`, which also predated EXIF orientation being applied before the dimensions are read. Only `sizeBytes` is measured unconditionally. The README claimed all four derived fields could not drift, and the glossary defined a derived field as one no draft may supply, both of which contradict the draft schema. AGENTS.md ships in the npm tarball and now links the glossary, so add CONTEXT.md to the package files list rather than shipping a broken link.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a65014524
ℹ️ 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".
|
🎉 This PR is included in version 1.3.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The repo had no single answer for what a reference, an entry, an asset, and a source each are. The docs used "screenshot" for all four, and the code carried a fourth vocabulary of its own.
Changes
CONTEXT.mdis the one glossary: 21 terms in four clusters, each with the wordings to avoid. Linked from the README docs list andAGENTS.md. It stays out of the npm tarball, likeAGENTS.mdandCONTRIBUTING.mdalready do.viewportis corrected everywhere it was described. The field records the CSS viewport a reference represents; the derived value measures the stored pixels. Those agree only at a device pixel ratio of 1, so "pass it for retina captures" was too narrow. Fixed indocs/VREF.md,skills/vref/SKILL.md,skills/vref/references/manifest.md, and thevref describenote agents read. The README now sayssizeBytescomes from the encoded byte length andviewportfrom the encoded pixel dimensions, rather than blurring both into "the encoded image".vrefDiris renamed tomanifestDir. It is the directory holding the manifest, which is.vref/only until--manifest docs/visual/manifest.jsonmoves it — a supported case the old name contradicted. Internal only; not part of theindex.tssurface.No behavior changes, so no release.
Left alone
convertGalleryconverts no gallery — it rewrites assets and manifest entries and never touchesindex.html. It is exported fromsrc/index.ts, so the honest name is a breaking change and not this PR's business.