fix(tlmviewer): stop probing targets_modified with requests that 404 - #3817
Conversation
FILECHECKSUM, FILEDISPLAY and the canvas image widgets located a target file by requesting it from targets_modified and treating the 404 as "not modified". That works, but the browser logs every failed request to the console regardless of how the code handles it - Ignore-Errors only suppresses our own toast - so an unmodified target produced a wall of red for a case that isn't an error. Add a TargetFiles mixin that asks /targets/:id/modified_files instead: one request per target rather than one failed request per file, and it says where every file lives. Widget.js mixes it in so any widget can use fetchTargetFile(). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
modified_files and all_modified only listed the local directory when OPENC3_LOCAL_MODE was set, so a file placed directly in the bucket was never reported as modified and widgets read the installed copy instead. Always list the bucket and union it with the local listing, and prefix the local results with the target name so both branches return the same scope relative names. Also fixes all_modified using a hardcoded DEFAULT prefix rather than the requested scope. Document that FILEDISPLAY and FILECHECKSUM read from the bucket, so a local mode file that exists only on disk can't be read. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The widget note belongs in widgets.yaml, which it is already in. The markdown is generated from it and says so at the top, so the edit would be overwritten by the next doc build. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3817 +/- ##
==========================================
- Coverage 79.44% 79.43% -0.02%
==========================================
Files 897 898 +1
Lines 67680 67691 +11
Branches 2664 2616 -48
==========================================
+ Hits 53766 53767 +1
- Misses 13248 13255 +7
- Partials 666 669 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The new TargetFiles mixin currently suppresses/absorbs server errors (e.g., 500) in a way that can silently return incorrect file versions, and ImageLoader has a prop option typo that disables required-prop validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reduces noisy browser console 404s in TlmViewer-related widgets by replacing per-file “probe targets_modified then fall back” requests with a single per-target lookup of modified files, then fetching the correct object directly from either targets or targets_modified. It also aligns backend modified-file listing behavior to consistently include bucket state even in local mode, and adds test coverage for the updated semantics.
Changes:
- Add a
TargetFilesmixin providingfetchTargetFile()andtargetFileRoot()based on/targets/:id/modified_files, and mix it into the sharedWidget(plusImageLoader). - Update FILEDISPLAY/FILECHECKSUM widgets to use the shared target-file fetch helper and clear cached modified-file state on refresh/reload.
- Update
TargetModel.modified_files/all_modifiedbehavior and add specs to cover scope-relative naming and local+bucket combination.
File summaries
| File | Description |
|---|---|
| openc3/spec/models/target_model_spec.rb | Adds specs validating modified file path shapes and local+bucket combination behavior. |
| openc3/lib/openc3/models/target_model.rb | Ensures bucket listing is always considered and normalizes/uniqs modified file outputs. |
| openc3/data/config/widgets.yaml | Documents FILEDISPLAY/FILECHECKSUM file sourcing behavior and local mode caveats. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/widgets/Widget.js | Mixes in TargetFiles so all widgets can reuse target file resolution. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/widgets/TargetFiles.js | New mixin: per-target modified-file caching and fetchTargetFile() helper. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/widgets/index.js | Exports the new TargetFiles mixin from the widget package. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/widgets/ImageLoader.js | Switches to modified-file lookup to avoid existence-probe 404s; updates header year. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/widgets/FiledisplayWidget.vue | Replaces manual modified-vs-installed probing with fetchTargetFile(). |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/widgets/FilechecksumWidget.vue | Replaces manual probing with fetchTargetFile() and clears cache on refresh. |
Review details
Suppressed comments (1)
openc3-cosmos-init/plugins/packages/openc3-vue-common/src/widgets/ImageLoader.js:27
- Vue prop validation uses
required: true, notrequire: true. With the current key, the prop isn't actually marked required, so missingtargetwon't be caught.
props: {
target: {
type: String,
require: true,
},
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| this.modifiedTargetFiles[targetName] = Api.get( | ||
| `/openc3-api/targets/${encodeURIComponent(targetName)}/modified_files`, | ||
| { headers: { 'Ignore-Errors': '404,500' } }, | ||
| ) | ||
| // modified_files reports target relative paths, the same shape we | ||
| // were given. If we can't tell, assume the installed file. | ||
| .then((response) => new Set(response.data)) | ||
| .catch(() => new Set()) |
| targets[target_name]['modified'] = true if targets[target_name] | ||
| end | ||
| else | ||
| modified_targets = Bucket.getClient().list_files(bucket: ENV['OPENC3_CONFIG_BUCKET'], path: "DEFAULT/targets_modified/", only_directories: true) |
There was a problem hiding this comment.
This previously hardcoded DEFAULT in the path so no other scopes could get their modified files. I'm not sure how this was never noticed.
There was a problem hiding this comment.
Looks like only TargetsTab and PluginsTab calls this. But the PluginsTab is what gates the "you have modified files" check os if you had modified files in a non-DEFAULT scope and install a new plugin it silently drops the changes. This is only if you've disabled LOCAL_MODE.
There was a problem hiding this comment.
🟡 Changes recommended
Lookup failures can silently select stale files, while local-only modifications are incorrectly mapped to unavailable bucket objects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
openc3-cosmos-init/plugins/packages/openc3-vue-common/src/widgets/TargetFiles.js:51
- Do not turn every
modified_filesfailure into an empty set. A transient 500/network error then makes the widget read the installed copy and can silently display or checksum stale content even though a modified copy exists. This lookup is authoritative, so let failures propagate to the widgets' existing error handling rather than treating them as “not modified.”
.catch(() => new Set())
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
A failed listing resolved to an empty Set that stayed cached, downgrading every remaining file of that target to the installed copy for the life of the widget. ImageLoader never clears the cache, so nothing retried. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
delete_config only touches the filesystem when local mode is on and the local mode path exists, so the spec returned nil off CI. Point local mode at a temp dir and assert the file is actually removed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
clayandgen
left a comment
clayandgen
left a comment
There was a problem hiding this comment.
Replacing the mixin strategy with composables is probably a worthy update? Functionality looks good
Both branches independently made the tool config delete spec runnable outside the container. Kept main's before/after hooks and the "deletes without local mode" test, folded in this branch's stronger assertions (drive save_config rather than hand-writing the file), and dropped the now-unused set_local_mode_path helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
ryan-pratt
left a comment
ryan-pratt
left a comment
There was a problem hiding this comment.
Composable would be good, but I think it would require bigger changes outside the scope of this fix and should probably be done separately.
What changed
FILECHECKSUM, FILEDISPLAY and the canvas image widgets located a target file by requesting it from targets_modified and treating the 404 as "not modified". That works, but the browser logs every failed request to the console regardless of how the code handles it - Ignore-Errors only suppresses our own toast - so an unmodified target produced a wall of red for a case that isn't an error.
Add a TargetFiles mixin that asks /targets/:id/modified_files: one request per target rather than one failed request per file, and it says where every file lives. Widget.js mixes it in so any widget can use fetchTargetFile().
Why it changed
Console errors should only be for real errors
Testing strategy
Opened the filechecksum, filedisplay, and graphs screens and observed no more console errors