Repository navigation
Support media file assets - #70
Conversation
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. |
![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: 8d9307529c
ℹ️ 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".
| (let* ((source (pathname-designator-string source)) | ||
| (target-filename (pathname-designator-string | ||
| (or target-filename source))) |
There was a problem hiding this comment.
When SOURCE is an absolute pathname—such as the common result of asdf:system-relative-pathname—and :target-filename is omitted, this assigns that absolute namestring as the target. validate-asset then rejects it unconditionally, despite the public API describing :target-filename as optional and promising a source-derived relative target. Derive the default with the existing relative-path logic or require an explicit target for sources that cannot be relativized.
AGENTS.md reference: AGENTS.md:L9-L11
Useful? React with 👍 / 👎.
| (maphash (lambda (name other-asset) | ||
| (when (and (not (eq name (asset-name asset))) | ||
| (string= (asset-target-filename other-asset) | ||
| (asset-target-filename asset))) |
There was a problem hiding this comment.
For every asset occurrence, asset-to-local-image calls validate-asset, and this maphash scans the complete registry to detect a collision. A document with M asset references and N declarations therefore performs O(MN) validation, directly contradicting the committed [PERF] constraint that resolution must not scan the declaration set. Maintain a target-path index or validate collisions when registering declarations instead.
AGENTS.md reference: AGENTS.md:L9-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I do not think this is a big issue right now.
| (40ants-doc-full/commondoc/xref::extract-symbols document) | ||
| document)) | ||
| (document (40ants-doc-full/commondoc/xref:fill-locatives document)) | ||
| (document (40ants-doc-full/assets::replace-assets document)) |
There was a problem hiding this comment.
When the documented public setting *document-uppercase-is-code* is nil, the preceding branch leaves prose as text, so replace-assets receives no XREF nodes and every bare @NAME.PNG remains text. This makes DEFASSET unusable for configurations that disable implicit symbol formatting, although its contract says bare asset names become images; asset recognition needs to run independently of the generic uppercase-code pass.
AGENTS.md reference: AGENTS.md:L9-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think it would be useful to support an explicit inclusion on the asset as a section like this:
(defasset @demo.gif #"static/demo.gif")
(defsection @example ()
"Here is:"
@demo.gif)
There was a problem hiding this comment.
Implemented in 9573eb9. When implicit uppercase-code formatting is disabled, the XREF pass now extracts only registered assets; a bare @NAME.PNG remains an image.
There was a problem hiding this comment.
Implemented in 9573eb9. A declared asset symbol can now be a direct DEFSECTION entry, e.g. @demo.gif after a prose entry.
1 participant
Implementation for issue #43