Clean up viewmaster (WIP) #14
Conversation
…t setup; add documentation for running Viewmaster locally.
…ommand line testing setup.
… variable instead of reading HOLDINGS_PATHS in customized httpd conf to obtain the holdings path. Also remove the step of creating symlinks of the holdings under /Library/Webserver/Documents.
…_paths function in viewmaster.py and viewmaster-without-pause.py, streamlining the codebase further.
…repository and correcting the pip install command.
…d update comments in viewmaster.py to reflect the new method of obtaining HOLDINGS_PATHS from the PDS3_HOLDINGS environment variable.
WalkthroughCentralized, lazily-cached logging was added and threaded through public APIs; holdings-path discovery was simplified to an environment-driven single path; page-cache and initialization were made logger-aware; new grouping and iterator classes were introduced; Sphinx docs, README, and build tooling were added; viewmaster_config testing heuristics simplified. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (18)
viewmaster/viewmaster-without-pause.py (6)
886-889: cgi.escape is removed in modern Python; switch to html.escape.This will raise AttributeError on Python 3.8+. Replace cgi.escape with html.escape and import html.
-import cgi +import html @@ - if params['selection']: - anchor_suffix = '#' + cgi.escape(params['selection'], quote=True) + if params['selection']: + anchor_suffix = '#' + html.escape(params['selection'], quote=True) @@ - if params['selection']: - selection_escaped = cgi.escape(params['selection'], quote=True) + if params['selection']: + selection_escaped = html.escape(params['selection'], quote=True)Also applies to: 1678-1693
1712-1714: Fix ValidationError import and reference.
wtforms.validators.ValidationErroris used butwtformsisn’t imported; this will NameError. Import the symbol directly and reference it.-from wtforms import StringField, HiddenField +from wtforms import StringField, HiddenField +from wtforms.validators import ValidationError @@ - if matchobj is None: - raise wtforms.validators.ValidationError("") + if matchobj is None: + raise ValidationError("")Also applies to: 3-5
1493-1505: Bug: misspelled key and wrong return value.
defaults[keuy]andreturn clean_query_params(params)are incorrect.- new_params = params.copy() + new_params = params.copy() @@ - for key in defaults: - if key not in new_params: - new_params[key] = defaults[keuy] + for key in defaults: + if key not in new_params: + new_params[key] = defaults[key] - return clean_query_params(params) + return clean_query_params(new_params)
522-526: Undefined variable row_pdsfile.Should copy the current query pdsfile, not an undefined name. This currently crashes on the nonexistent index-row path.
- row_pdsfile = row_pdsfile.copy() + row_pdsfile = query_pdsfile.copy()
297-305: Consider gating symlink logic and unused function.
create_holdings_symlinksremains but is no longer used. Either remove or clearly mark as legacy to avoid confusion.
203-205: Prefer LOGGER.warning over warn.If pdslogger mirrors stdlib, use
warning()to align with modern APIs.Also applies to: 297-305, 297-305
viewmaster/viewmaster_config.py (2)
32-39: Testing config overridden on Linux (memcached sockets).On Linux, the block unconditionally sets memcache sockets, overriding the earlier testing “0” settings. This enables memcache during tests unintentionally.
-if platform.system() == 'Linux': - VIEWMASTER_MEMCACHE_PORT = '/var/run/memcached/memcached.socket' - PDSFILE_MEMCACHE_PORT = '/var/run/memcached/memcached.socket' +if platform.system() == 'Linux': + if VIEWMASTER_TESTING: + VIEWMASTER_MEMCACHE_PORT = 0 + PDSFILE_MEMCACHE_PORT = 0 + else: + VIEWMASTER_MEMCACHE_PORT = '/var/run/memcached/memcached.socket' + PDSFILE_MEMCACHE_PORT = '/var/run/memcached/memcached.socket'Also applies to: 40-46
24-31: Re-evaluate MAKE_SYMLINKS defaults.If symlink creation is removed from startup, keeping MAKE_SYMLINKS=True in production may be misleading.
viewmaster/viewmaster.py (10)
31-33: Do not hard‑code Flask secret keys.Load from an environment variable and fail closed in non‑testing; hard‑coded secrets are a security risk.
-app = Flask(__name__) -app.secret_key = "Cassini Grand Finale!" # needed by flask_wtf +app = Flask(__name__) +# Load secret from env; in testing allow a default, but never check in real keys +app.config['SECRET_KEY'] = os.getenv('VIEWMASTER_SECRET_KEY', 'dev-only')
1-1: Remove duplicate import.
redirectis imported twice.-from flask import Flask, flash, redirect, render_template, redirect, request, send_file +from flask import Flask, flash, redirect, render_template, request, send_file
360-363: Remove stray print.Debug print leaks noise in logs/STDOUT.
- LOGGER.replace_root(HOLDINGS_PATHS) - print(VIEWMASTER_PREFIX_+ICON_URL_) + LOGGER.replace_root(HOLDINGS_PATHS)
933-936: Replace cgi.escape with html.escape.
cgi.escapeis removed in modern Python; this will break at runtime.-import cgi +import html @@ - if params['selection']: - anchor_suffix = '#' + cgi.escape(params['selection'], quote=True) + if params['selection']: + anchor_suffix = '#' + html.escape(params['selection'], quote=True) @@ - if params['selection']: - selection_escaped = cgi.escape(params['selection'], quote=True) + if params['selection']: + selection_escaped = html.escape(params['selection'], quote=True)Also applies to: 1779-1781, 1791-1793, 2151-2170
1818-1820: Fix ValidationError import and usage.Avoid NameError by importing the symbol directly.
-from wtforms import StringField, HiddenField +from wtforms import StringField, HiddenField +from wtforms.validators import ValidationError @@ - if matchobj is None: - raise wtforms.validators.ValidationError("") + if matchobj is None: + raise ValidationError("")Also applies to: 1804-1816
1599-1605: Bug: misspelled key and wrong return value.Fix
keuytypo and return the cleaned new_params.- for key in defaults: - if key not in new_params: - new_params[key] = defaults[keuy] - - return clean_query_params(params) + for key in defaults: + if key not in new_params: + new_params[key] = defaults[key] + + return clean_query_params(new_params)
560-566: Undefined variable row_pdsfile.This branch currently crashes; copy the current query file instead.
- row_pdsfile = row_pdsfile.copy() + row_pdsfile = query_pdsfile.copy()
1844-1851: Path traversal risk in test routes; gate by testing flag.Routes serve arbitrary filesystem paths without confinement. Restrict to a directory via send_from_directory and only register in testing.
-from flask import ... , send_file +from flask import ... , send_from_directory @@ -@app.route('/icons-local/<path:query_path>') -def return_icons_local(query_path): - return send_file(f'../icons/{query_path}', mimetype='image/png') +if VIEWMASTER_TESTING: + @app.route('/icons-local/<path:query_path>') + def return_icons_local(query_path): + return send_from_directory('../icons', query_path, mimetype='image/png') @@ -@app.route('/holdings/<path:query_path>') -def return_holdings_local(query_path): - return send_file(f'{HOLDINGS_PATHS[0]}/{query_path}', mimetype='text/plain') + @app.route('/holdings/<path:query_path>') + def return_holdings_local(query_path): + return send_from_directory(HOLDINGS_PATHS[0], query_path)Also applies to: 1848-1851
1714-1716: Duplicate FILTER_REGEX definitions.There are two different FILTER_REGEX patterns. Consolidate to one to avoid confusion.
Also applies to: 1802-1804
305-313: Single holdings path assertion.You assert a single holdings path. If future configs need multiple paths, this will break. Consider logging a warning instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
viewmaster/doc/viewmaster_v1.md(1 hunks)viewmaster/viewmaster-without-pause.py(4 hunks)viewmaster/viewmaster.py(1 hunks)viewmaster/viewmaster_config.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
viewmaster/viewmaster.py (1)
viewmaster/viewmaster-without-pause.py (1)
get_holdings_paths(139-146)
🪛 markdownlint-cli2 (0.18.1)
viewmaster/doc/viewmaster_v1.md
1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
8-8: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
8-8: Multiple headings with the same content
(MD024, no-duplicate-heading)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
19-19: Multiple headings with the same content
(MD024, no-duplicate-heading)
20-20: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
20-20: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
22-22: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (1)
viewmaster/viewmaster.py (1)
1897-1905: Pause/resume caching commented out.Confirm updated Pds3File caching semantics. Consider removing or updating to the new API to avoid stale caches.
Also applies to: 1976-1979
viewmaster/doc/viewmaster_v1.md
Outdated
| - `python -m venv myenv` (Replace `myenv` with the desired name for your virtual environment) | ||
| - `source myenv/bin/activate` | ||
| - `pip install -r requirements.txt` | ||
| - Create an environement variable `PDS3_HOLDINGS` to store the path of PDS3 holdings. |
There was a problem hiding this comment.
Typos and requirement clarity.
- “environement” → “environment”.
- Clarify the path must end with “/holdings” to pass validation.
🤖 Prompt for AI Agents
In viewmaster/doc/viewmaster_v1.md around line 17, fix the typo and clarify the
requirement: change “environement” to “environment” and update the sentence to
state that the PDS3_HOLDINGS environment variable should store the path to the
PDS3 holdings and that the path must end with “/holdings” to pass validation.
viewmaster/doc/viewmaster_v1.md
Outdated
| - Execute viewmaster.py in the terminal: | ||
| - `sudo -E python viewmaster/viewmaster.py` | ||
| - Open your browser and enter `http://127.0.0.1:8080/` (this is the value assigned to VIEWMASTER_PREFIX_ in viewmaster_config.py). Now the testing viewmaster will be running on your browser. |
There was a problem hiding this comment.
❓ Verification inconclusive
Avoid sudo for local runs.
Port 8080 doesn’t require root; recommending sudo -E is unnecessary and risky. Use the regular interpreter; if env propagation is needed, export before running.
Remove sudo from local run instructions (viewmaster/doc/viewmaster_v1.md:21-23)
Use python viewmaster/viewmaster.py instead of sudo -E python…; export any needed environment variables beforehand.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
22-22: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
In viewmaster/doc/viewmaster_v1.md around lines 21 to 23, remove the use of sudo
in the local run instructions: replace `sudo -E python viewmaster/viewmaster.py`
with `python viewmaster/viewmaster.py` and add a short note to export any
required environment variables beforehand (e.g., `export VAR=value`) so users
run the script without elevated privileges.
viewmaster/viewmaster_config.py
Outdated
| # Test for a command line run, sudo python viewmaster/viewmaster.py | ||
| VIEWMASTER_TESTING = 'viewmaster.py' in sys.argv[0] | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make testing mode explicit and less brittle.
Relying on 'viewmaster.py' in sys.argv[0] is fragile. Consider an env var flag and/or CLI option.
-VIEWMASTER_TESTING = 'viewmaster.py' in sys.argv[0]
+VIEWMASTER_TESTING = os.getenv('VIEWMASTER_TESTING', '0') == '1'Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Leave as-is, this is for manually running viewmaster script in the command line during development
There was a problem hiding this comment.
@juzen2003, understood! I'll leave the testing detection as-is for your command-line development workflow.
(„• ֊ •„)
づ♡
…up instructions, adding an overview of key components, and detailing the core Python files and their functionalities. Update version information to October 2025.
…p instructions, and local running guidelines for Viewmaster.
…outlining the iterator classes, their purposes, key features, and design patterns for navigating PDS file structures.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)viewmaster/doc/viewmaster_v1.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1169 characters long)
Context: ...--- | ------------ | -------------- | | 
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
viewmaster/doc/viewmaster_v1.md
19-19: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
24-24: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
37-37: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
49-49: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
59-59: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
64-64: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
67-67: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
78-78: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
131-131: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
134-134: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
155-155: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
158-158: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
180-180: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
183-183: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
196-196: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
199-199: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
214-214: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
217-217: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
219-219: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
220-220: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
221-221: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
222-222: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
223-223: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
224-224: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
225-225: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
226-226: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
227-227: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
230-230: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
231-231: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
232-232: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
233-233: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
234-234: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
235-235: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
236-236: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
237-237: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
238-238: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
241-241: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
242-242: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
243-243: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
244-244: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
245-245: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
248-248: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
README.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
18-18: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
23-23: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
36-36: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (1)
README.md (1)
1-43: Content aligns well with PR objectives.The documentation correctly reflects the shift from Apache/httpd configuration parsing to environment-variable-based holdings path management (
PDS3_HOLDINGS), and removes references to symlink creation. The setup and local-run instructions are clear and actionable.
viewmaster/doc/viewmaster_v1.md
Outdated
| # Viewmaster Help Guide | ||
|
|
||
| Version 1.0 | ||
| October 2025 | ||
|
|
||
| # Viewmaster | ||
|
|
||
| ## Overview | ||
|
|
||
| **Viewmaster** is a web-based file and document viewer and management tool. It is focused on handling and displaying various types of PDS documents and folders. | ||
|
|
||
| --- | ||
|
|
||
| # Running Viewmaster | ||
|
|
||
| ## Environment Setup (first-time only) | ||
|
|
||
| 1. Clone the repository: | ||
| ```bash | ||
| git clone https://github.com/SETI/rms-viewmaster.git | ||
| ``` | ||
|
|
||
| 2. Create a virtual environment and install dependencies in the `rms-viewmaster` directory: | ||
| ```bash | ||
| python -m venv myenv # Replace "myenv" with your preferred name | ||
| source myenv/bin/activate | ||
| pip install -r requirements.txt | ||
| ``` | ||
|
|
||
| 3. Set the environment variable `PDS3_HOLDINGS` to the path of your PDS3 holdings. | ||
|
|
||
| --- | ||
|
|
||
| ## Running Locally | ||
|
|
||
| 1. Start the server: | ||
| ```bash | ||
| sudo -E python viewmaster/viewmaster.py | ||
| ``` | ||
|
|
||
| 2. Open your browser and go to: | ||
| [http://127.0.0.1:8080/](http://127.0.0.1:8080/) | ||
| *(This corresponds to `VIEWMASTER_PREFIX_` in `viewmaster_config.py`.)* | ||
|
|
||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Fix markdown lint violations in setup section.
The document contains multiple linting violations flagged in static analysis and a previous review. Key issues include missing blank lines around headings (MD022), incorrect code block spacing (MD031), and list indentation inconsistencies (MD007). These should be corrected throughout the document, particularly in the setup and running sections.
For example:
- Add blank lines before/after heading on line 1 and line 6
- Add blank lines before/after code blocks on lines 19, 24, 37
- Verify list indentation aligns with markdown standards
Generate a script to identify and fix the remaining markdown lint issues:
🏁 Script executed:
#!/bin/bash
# Description: Run markdownlint on the file to identify remaining issues
markdownlint-cli2 viewmaster/doc/viewmaster_v1.mdLength of output: 12709
Comprehensive markdown linting fixes required throughout the document.
The markdownlint scan identified 103 errors spanning the entire document, not just the setup sections. Address the following violations:
- MD025 (multiple h1 headings): Lines 6, 14, 47, 74 — restructure to single top-level heading
- MD031 (blanks around code blocks): Lines 19, 24, 37 — add blank lines before/after fenced code blocks
- MD022 (blanks around headings): Lines 49, 59, 64, 67, 78, 131, 134, 155, 158, 180, 183, 196, 199, 214, 217 — add blank lines before/after headings
- MD007 (list indentation): Lines 219–227, 230–238, 241–245, 248 — fix indentation to 2 spaces per level
- MD013 (line length): Many lines exceed 80 character limit — wrap or reformat
- MD032 (blanks around lists): Lines 50, 60, 65, 68, 135, 159, 184, 200, 218 — add blank lines before/after lists
- MD024 (duplicate headings): Multiple instances of "Overview," "Key Components" — use unique headings
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
19-19: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
24-24: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
37-37: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
In viewmaster/doc/viewmaster_v1.md around lines 1–45, the document fails many
markdownlint rules (MD025, MD031, MD022, MD007, MD013, MD032, MD024) —
consolidate multiple H1s into a single top-level heading, ensure single blank
lines before and after all headings and fenced code blocks, add blank lines
around lists, normalize list indentation to 2 spaces per level, wrap or reflow
long lines to keep under ~80 characters, and rename or merge duplicate section
headings (e.g., "Overview", "Key Components") so each heading is unique; apply
these changes consistently across the file to resolve the reported lint
violations.
viewmaster/doc/viewmaster_v1.md
Outdated
| pip install -r requirements.txt | ||
| ``` | ||
|
|
||
| 3. Set the environment variable `PDS3_HOLDINGS` to the path of your PDS3 holdings. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify that PDS3_HOLDINGS path must end with /holdings.
The instruction should explicitly state that the path must end with /holdings to pass validation. This requirement was noted in a previous review and should be included for clarity.
-3. Set the environment variable `PDS3_HOLDINGS` to the path of your PDS3 holdings.
+3. Set the environment variable `PDS3_HOLDINGS` to the absolute path of your PDS3 holdings directory (must end with `/holdings`):
+
+ ```bash
+ export PDS3_HOLDINGS=/absolute/path/to/holdings
+ ```🤖 Prompt for AI Agents
In viewmaster/doc/viewmaster_v1.md around line 30, clarify that the
PDS3_HOLDINGS environment variable must point to the absolute path that ends
with "/holdings" so validation will pass; update the text to explicitly state
the trailing "/holdings" requirement and adjust the example export line to show
an absolute path ending with "/holdings".
viewmaster/doc/viewmaster_v1.md
Outdated
|
|
||
| 1. Start the server: | ||
| ```bash | ||
| sudo -E python viewmaster/viewmaster.py |
There was a problem hiding this comment.
🧩 Analysis chain
CRITICAL: Remove sudo -E from local run instructions.
Port 8080 does not require root privileges. Using sudo -E is unnecessary and introduces a security risk. This issue was flagged in a previous review and should be corrected before merge.
1. Start the server:
```bash
- sudo -E python viewmaster/viewmaster.py
+ python viewmaster/viewmaster.py
```If environment variable propagation is needed, export variables before executing:
export PDS3_HOLDINGS=/path/to/holdings
python viewmaster/viewmaster.pyI'll verify the claims in this review comment by examining the file and the underlying code.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the file content around line 38
cat viewmaster/doc/viewmaster_v1.md | head -50 | tail -20
Length of output: 357
🏁 Script executed:
#!/bin/bash
# Search for port configuration in viewmaster.py
rg -n "port|8080|listen" viewmaster/viewmaster.py -C2Length of output: 1497
🏁 Script executed:
#!/bin/bash
# Check if viewmaster.py requires elevated privileges or uses ports < 1024
rg -n "sudo|root|privilege|:80\b|:443\b|:8080" viewmaster/viewmaster.pyLength of output: 530
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for file system operations that might require elevated privileges
rg -n "open\(|mkdir|symlink|chmod|chown|mount|rmtree" viewmaster/viewmaster.py -C1
Length of output: 2577
🏁 Script executed:
#!/bin/bash
# Check viewmaster_config.py to see what environment setup is needed
cat viewmaster/viewmaster_config.py | head -100Length of output: 1802
🏁 Script executed:
#!/bin/bash
# Look for any explicit privilege checks or warnings in the documentation
rg -n "sudo|root|privilege|elevation" viewmaster/doc/viewmaster_v1.mdLength of output: 45
CRITICAL: Remove sudo -E from local run instructions.
Port 8080 does not require root privileges. Using sudo -E is unnecessary and introduces a security risk. This issue was flagged in a previous review and should be corrected before merge.
1. Start the server:
```bash
- sudo -E python viewmaster/viewmaster.py
+ python viewmaster/viewmaster.py
```If environment variable propagation is needed, export variables before executing:
export PDS3_HOLDINGS=/path/to/holdings
python viewmaster/viewmaster.py🤖 Prompt for AI Agents
In viewmaster/doc/viewmaster_v1.md around line 38, the local run instruction
uses "sudo -E python viewmaster/viewmaster.py" which is unnecessary and
insecure; remove "sudo -E" so the command runs as "python
viewmaster/viewmaster.py" and update the surrounding guidance to show exporting
any required environment variables beforehand (e.g., include a short note or
example showing export PDS3_HOLDINGS=... followed by running the python
command).
viewmaster/doc/viewmaster_v1.md
Outdated
| # Key Components | ||
|
|
||
| ## 1. Main Application Code (`viewmaster/`) | ||
| - **Python Modules:** | ||
| - `link.py`, `pdsgroup.py`, `pdsgrouptable.py`, `pdsiterator.py`: Handle core logic for grouping, iterating, and linking PDS documents or datasets. | ||
| - `viewmaster.py`, `viewmaster-without-pause.py`: Main entry points and application logic for the viewer. | ||
| - `viewmaster_config.py`: Configuration settings for the application. | ||
|
|
||
| - **Templates (`viewmaster/templates/`):** | ||
| - HTML files for rendering views: tables, grids, navigation, error pages, feedback, etc. | ||
| - These templates provide the web interface, including navigation, table/grid views, and feedback/error handling. | ||
|
|
||
| ## 2. Static Assets (`icons/`) | ||
| - Organized by color (black, blue, gray) and size (30, 50, 100, 200, 500). | ||
| - Icons for document types (archive, binary, book, image, PDF, software, etc.) and folders (archives, binary, cubes, diagrams, etc.). | ||
| - Used for visually representing files and folders in the UI. | ||
|
|
||
| ## 3. Web Server Integration | ||
| - `viewmaster.wsgi`, `link.wsgi`, `wsgi_init.py`: WSGI entry points for deploying the application with a WSGI-compatible web server. | ||
|
|
||
| ## 4. Project Metadata | ||
| - `README.md`: Project overview and instructions. | ||
| - `requirements.txt`: Python dependencies. | ||
| - `LICENSE`, `CODE_OF_CONDUCT.md`, `CONTRIBUTING.md`: Standard open-source project files. | ||
|
|
||
| --- | ||
|
|
||
| # Summary of the Core Python Files | ||
|
|
||
| ## `viewmaster.py` and `viewmaster-without-pause.py` | ||
|
|
||
| ### Overview | ||
| `viewmaster.py` is the core module of the application. It sets up a Flask-based server that serves as a document viewer for PDS files. Users can browse, view, and navigate through a hierarchical structure of files and folders, with support for multiple document types and metadata. | ||
|
|
||
| `viewmaster-without-pause.py` is a variant designed to run without pausing or waiting for user input. | ||
|
|
||
| ### Key Components and Functionality | ||
|
|
||
| 1. **Imports and Initialization** | ||
| - Imports Flask and related modules (FlaskForm, StringField, HiddenField) for web handling and form processing. | ||
| - Imports utility modules (os, sys, logging, psutil, etc.) and custom modules (pdsfile, pdsiterator, pdslogger, pdstable, pdsgroup, pdsgrouptable). | ||
| - Initializes the Flask app with a secret key and configuration values (like `LOCAL_IP_ADDRESS`). | ||
| - Imports configuration parameters (e.g., `LOCALHOST_`, `VIEWMASTER_PREFIX_`, `WEBSITE_HTTP_HOME`) from `viewmaster_config.py`. | ||
| - Configures logging with `pdslogger`, with separate log files for info and debug levels. | ||
|
|
||
| 2. **Constants and Configuration** | ||
| - Defines constants such as `ICON_ROOT_`, `ICON_URL_`, and `VIEWABLE_EXTENSIONS`. | ||
| - Defines `ASSOCIATED_CATEGORIES` for navigation and grouping. | ||
| - Sets UI and navigation limits (`MAX_PAGES`, `MAX_NAV_STRLEN`, etc.). | ||
|
|
||
| 3. **Holdings Path Management** | ||
| - `get_holdings_paths()` reads the Apache configuration file (or environment variable) to determine the holdings directories. | ||
| - `validate_holdings_paths()` checks for existence and logs warnings/errors. | ||
| - `create_holdings_symlinks()` likely creates symbolic links for easier access. | ||
|
|
||
| 4. **Cache Initialization** | ||
| - Functions like `initialize_caches()`, `build_cache()`, and `reset_cache()` manage caching for performance. | ||
| - Uses memcached (via `pylibmc`) for caching. | ||
|
|
||
| 5. **Navigation and Page Generation** | ||
| - Functions such as `get_prev_next_navigation()`, `list_next_pdsfiles()`, and `fill_level_navigation_links()` generate navigation links. | ||
| - `get_directory_page()` and `directory_page_html()` render directory listings. | ||
| - `get_product_page_info()` and `product_page_html()` render product pages. | ||
|
|
||
| 6. **URL Parameter Handling** | ||
| - Functions like `get_query_params_from_request()`, `get_query_params_from_url()`, and `clean_query_params()` parse and sanitize URL parameters. | ||
| - `url_params()` constructs URLs with the proper parameters. | ||
|
|
||
| 7. **Form Handling** | ||
| - Defines a `FilterForm` class (Flask-WTF) for file name filtering. | ||
| - The `/set_filter` route processes POST requests to update filters. | ||
|
|
||
| 8. **Route Definitions** | ||
| - The main route (`@app.route('/<path:query_path>')`) handles all incoming requests. | ||
| - Additional routes handle filtering, serving icons, holdings, and feedback. | ||
|
|
||
| 9. **Utility Functions** | ||
| - Functions like `format_row_value()` and `format_tuple()` prepare data for display. | ||
| - `trim_html()` likely cleans up HTML for rendering. | ||
|
|
||
| --- | ||
|
|
||
| ## `viewmaster_config.py` | ||
|
|
||
| ### Overview | ||
| `viewmaster_config.py` is the configuration module for Viewmaster. It defines environment-specific settings (URLs, logging, caching) based on whether the app is in testing, local, or production mode. | ||
|
|
||
| ### Key Components and Functionality | ||
| 1. **Environment Detection** | ||
| - Uses `platform` and `socket` to detect OS and hostname. | ||
| - Flags like `VIEWMASTER_TESTING` and `VIEWMASTER_FOR_MARK` determine the environment. | ||
|
|
||
| 2. **Configuration Blocks** | ||
| - **Testing**: Localhost URLs, caching disabled, log name `pds.viewmaster.testing`. | ||
| - **Local**: Custom localhost URLs, symlinks enabled, caching disabled, log name `pds.viewmaster.local`. | ||
| - **Production**: Deployment URLs, symlinks enabled, caching disabled, log name `pds.viewmaster.server`. | ||
|
|
||
| 3. **Platform-Specific Settings** | ||
| - Linux: uses `/var/run/memcached/memcached.socket`, `/var/www/` paths, Apache config in `/etc/apache2/`. | ||
| - macOS: uses `/var/tmp/memcached.socket`, `/Library/WebServer/` paths, Apache config in `/usr/local/etc/httpd/`. | ||
|
|
||
| 4. **Other Settings** | ||
| - `USE_SHELVES_ONLY = False` (application supports multiple storage mechanisms). | ||
|
|
||
| --- | ||
|
|
||
| ## `link.py` | ||
|
|
||
| ### Overview | ||
| `link.py` is a Flask-based microservice for URL redirection. It redirects requests either to the main Viewmaster app (for directories) or directly to files. | ||
|
|
||
| ### Key Components and Functionality | ||
| 1. **Imports and Initialization** | ||
| - Uses Flask (redirect, abort), os, glob, logging, and `pdslogger`. | ||
| - Imports configuration from `viewmaster_config.py`. | ||
|
|
||
| 2. **Logging Configuration** | ||
| - Log name derived from Viewmaster config, replacing "viewmaster" with "link". | ||
| - Configured for rotating log files. | ||
|
|
||
| 3. **Route Definitions** | ||
| - Main route: `/path:query_path>` defaults to `volumes`. | ||
| - Determines whether to redirect to a directory view or directly to a file. | ||
|
|
||
| 4. **Redirection Logic** | ||
| - If a directory: redirects to `VIEWMASTER_PREFIX_ + query_path`. | ||
| - If a file: finds file via glob, then redirects to `WEBSITE_HTTP_HOME + relative path`. | ||
| - If missing: logs error and returns 404. | ||
|
|
||
| --- | ||
|
|
||
| ## `pdsgroup.py` | ||
|
|
||
| ### Overview | ||
| `pdsgroup.py` defines the `PdsGroup` class for managing an ordered set of `PdsFile` objects. Groups related files (e.g., a file and its label) for display in the UI. | ||
|
|
||
| ### Key Components | ||
| - **Initialization**: Stores parent directory, anchor, and files. | ||
| - **Methods**: | ||
| - `__len__`, `__repr__`, `copy()`. | ||
| - Sorting (`sort()`), grouping (`group_children()`). | ||
| - File management (`append()`, `remove()`, `hide()`, `unhide()`). | ||
| - Iterators for visible/hidden/all files. | ||
| - **Icons/Viewsets**: Manages closed/open states and viewable sets. | ||
|
|
||
| --- | ||
|
|
||
| ## `pdsgrouptable.py` | ||
|
|
||
| ### Overview | ||
| `pdsgrouptable.py` defines the `PdsGroupTable` class for managing an ordered set of `PdsGroup` objects. It organizes groups into tables for display. | ||
|
|
||
| ### Key Components | ||
| - **Initialization**: Stores groups and parent. | ||
| - **Methods**: | ||
| - `__repr__`, `copy()`. | ||
| - Insert groups/files (`insert_group`, `insert_file`, `insert()`). | ||
| - Sorting (`sort_in_groups`, `sort_groups`). | ||
| - File/group hiding/removal. | ||
| - Iterators for groups/files. | ||
| - **Static Methods**: | ||
| - `sort_tables()`, `tables_from_pdsfiles()`, `merge_index_row_tables()`. | ||
|
|
||
| --- | ||
|
|
||
| ## `pdsiterator.py` | ||
|
|
||
| ### Overview | ||
| `pdsiterator.py` provides three main iterator classes for navigating through PDS file structures. | ||
|
|
||
| ### Key Components and Functionality | ||
| - **PdsDirIterator** (lines 29-144) | ||
| - **Purpose**: Iterates across related directories, jumping into adjacent directories and parallel volumes | ||
| - **Key Features**: | ||
| - Uses a global `DIRECTORY_CACHE` to optimize performance by caching directory listings | ||
| - Supports both forward and backward iteration (via `sign` parameter) | ||
| - Handles case-insensitive path matching | ||
| - Returns tuples of `(logical_path, display_path, level)` where: | ||
| - `level = 0`: same directory level | ||
| - `level = 1`: different directory level | ||
| - Uses `pdsfile.NEIGHBORS` to find related directories using fnmatch patterns | ||
|
|
||
| - **PdsFileIterator** (lines 150-268) | ||
| - **Purpose**: Iterates through files within directories, with support for jumping to adjacent directories when needed | ||
| - **Key Features**: | ||
| - Supports pattern matching and filtering of file names | ||
| - Can exclude files based on patterns | ||
| - Supports custom filter functions | ||
| - Handles "cousin" navigation (jumping to adjacent parent directories) | ||
| - Returns tuples with level indicators: | ||
| - `level = 0`: sibling files (same directory) | ||
| - `level = 1`: cousin files (different directory) | ||
|
|
||
| - **PdsRowIterator** (lines 274-336) | ||
| - **Purpose**: Simple iterator for files within a single directory (siblings only) | ||
| - **Key Features**: | ||
| - More lightweight than `PdsFileIterator` | ||
| - Only iterates within the same parent directory | ||
| - Always returns `level = 0` since it doesn't cross directory boundaries | ||
|
|
||
| - **Utility Functions:** | ||
| - **`dirs_only()`** (lines 12-16): Filter function that only returns directories | ||
|
|
||
| - **Key Design Patterns:** | ||
| 1. **Caching**: Uses `DIRECTORY_CACHE` to avoid repeated expensive directory operations | ||
| 2. **Case-insensitive matching**: All path comparisons are done in lowercase | ||
| 3. **Iterator protocol**: All classes implement `__iter__()`, `__next__()`, and `next()` methods | ||
| 4. **Cloning**: Each iterator has a `copy()` method for creating reversed or modified versions | ||
| 5. **Flexible navigation**: Supports both forward (`sign=1`) and backward (`sign=-1`) iteration | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extended documentation is comprehensive but contains formatting issues.
The detailed breakdown of Key Components, Core Python Files, and module documentation (lines 47-256) is thorough and helpful. However, static analysis flags multiple list indentation issues (MD007) throughout these sections. Consider running a markdown linter to fix all indentation to the standard 2-space increment and ensure blank lines separate headings as per MD022.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
49-49: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
59-59: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
64-64: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
67-67: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
78-78: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
131-131: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
134-134: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
155-155: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
158-158: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
180-180: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
183-183: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
196-196: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
199-199: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
214-214: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
217-217: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
219-219: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
220-220: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
221-221: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
222-222: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
223-223: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
224-224: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
225-225: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
226-226: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
227-227: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
230-230: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
231-231: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
232-232: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
233-233: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
234-234: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
235-235: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
236-236: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
237-237: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
238-238: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
241-241: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
242-242: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
243-243: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
244-244: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
245-245: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
248-248: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
In viewmaster/doc/viewmaster_v1.md around lines 47 to 256 there are multiple
markdown list indentation errors (MD007) and missing blank lines after headings
(MD022); run a markdown linter (e.g., markdownlint) and fix by normalizing list
indentation to 2-space increments for each nesting level, ensure nested bullets
align under their parent items, and add a single blank line after each heading;
reflow any mis-indented blocks so headings are followed by a blank line and
lists consistently use 2-space indentation throughout.
|
A couple of initial comments:
Different LLMs have different skills. Claude is tuned for coding, not for English prose. On the other hand, ChatGPT is excellent at prose. I suggest that you write a bunch of messy documentation yourself, perhaps building on what Claude wrote for you. Don't worry too much about spelling, grammar, or structure. Just get all the details in. Then paste it in to ChatGPT with a prompt like: "You are an expert English editor, Python programmer, and web developer. Below is technical documentation for a web application. Your job is to edit this document into a fully readable technical description, properly organized with headings and subheadings, with correct English spelling and grammar. The target audience is an experienced Python developer who is not previously familiar with this code base but needs to become an expert. Keep all details in the original document. Also mark areas where there is ambiguity and make suggestions for ways in which the document can be further improved at both the detail and general level. The result should be in Markdown." |
|
Also I agree that you shouldn't be using |
…clude new pdsgroup module documentation.
Updated the logger name variable from LOGNAME to link_logger_name to better reflect its purpose. This change enhances code readability and maintains consistency in naming conventions within the logging implementation.
Introduced a module-level logger cache to prevent duplicate logger instances. The new get_or_create_logger function ensures that the logger is created only once, enhancing performance and maintaining consistent logging behavior across requests. Updated the link function to utilize this new caching mechanism.
…improve code clarity and maintainability.
Introduced a ValueError exception to be raised when neither Pds3File nor Pds4File can create a parent PdsFile from the given logical path. This change improves error reporting and enhances the robustness of the PdsFileIterator class.
… consistency and clarity
…e clarity and maintainability.
…ed accuracy in command-line detection
Introduced a get_or_create_logger function to ensure the logger instance is created only once, preventing duplicate handlers. Updated the main execution block to utilize this new caching mechanism, enhancing performance and maintaining consistent logging behavior.
Updated instances of logger.warn to logger.warning for consistency and clarity in logging messages throughout the viewmaster.py file. This change enhances the logging functionality and maintains uniformity in the codebase.
… messages Updated the exception handling in get_holdings_path to log a more descriptive message when failing to get or validate holdings paths. Additionally, modified the Memcache connection error handling to remove the exception variable, streamlining the warning message. These changes enhance the clarity and usefulness of log outputs.
…nd functionality Updated the handling of HOLDINGS_PATHS and PAGE_CACHE to ensure they are properly initialized and managed. Introduced global declarations for these variables in relevant functions, enhancing the organization of cache management. This change streamlines the caching process and improves code readability.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
viewmaster/viewmaster_config.py (1)
47-50: Redundant memcache port assignments for testing mode.Memcache ports are set to
0in the testing block (lines 47-48), then conditionally set again for non-Linux testing (lines 68-70). This duplication is harmless but could be simplified.Suggested consolidation
if VIEWMASTER_TESTING: LOCALHOST_ = '/' VIEWMASTER_PREFIX_ = 'http://127.0.0.1:8080/' WEBSITE_HTTP_HOME = 'http://localhost' LOGNAME = 'pds.viewmaster.testing' - VIEWMASTER_MEMCACHE_PORT = 0 - PDSFILE_MEMCACHE_PORT = 0 MAKE_SYMLINKS = False PAGE_CACHING = False # ... else: - if VIEWMASTER_TESTING: - VIEWMASTER_MEMCACHE_PORT = 0 - PDSFILE_MEMCACHE_PORT = 0 - else: - VIEWMASTER_MEMCACHE_PORT = '/var/tmp/memcached.socket' - PDSFILE_MEMCACHE_PORT = '/var/tmp/memcached.socket' + VIEWMASTER_MEMCACHE_PORT = '/var/tmp/memcached.socket' + PDSFILE_MEMCACHE_PORT = '/var/tmp/memcached.socket'Then set memcache ports to
0in theVIEWMASTER_TESTINGblock unconditionally.Also applies to: 68-73
viewmaster/pdsgroup.py (2)
296-403: Critical: Duplicatesort()method definition.The
sort()method is defined twice (lines 296-348 and lines 350-403). The second definition completely shadows the first. This appears to be a copy-paste error.Fix: Remove the duplicate method
- def sort(self, labels_after=None, dirs_first=None, dirs_last=None, - info_first=None): - """Sort the rows of this group. - - Parameters: - labels_after (bool|None): Place labels after their targets. - dirs_first (bool|None): Sort directories before files. - dirs_last (bool|None): Sort directories after files. - info_first (bool|None): Prioritize info files. - - Returns: - None - """ - - basename_dict = {pdsf.basename:pdsf for pdsf in self.rows} - - if self.parent_pdsf: - sorted = self.parent_pdsf.sort_basenames(list(basename_dict.keys()), - labels_after=labels_after, - dirs_first=dirs_first, - dirs_last=dirs_last, - info_first=info_first) - - # If there are multiple labels in the group, make sure each label - # is adjacent to any data files of the same name. This can comes up - # with Juno JIRAM, at least. - labels = [basename for basename in sorted - if self.parent_pdsf.basename_is_label(basename)] - if len(labels) > 1: - for label in labels: - l = sorted.index(label) # index of label - pattern = os.path.splitext(label)[0] + '.' - tlist = [t for t in range(len(sorted)) # indices of targets - if sorted[t].startswith(pattern)] - tlist.remove(l) - - # If the label is not adjacent, but could be... - if tlist and (l+1) not in tlist and (l-1) not in tlist: - # Locate the nearest target with a matching basename - diffs = [(abs(t-l),t) for t in tlist] - tbest = min(diffs)[1] - - # Relocate the label to an adjacent location - if l < tbest: - sorted.insert(tbest - 1, sorted.pop(l)) - else: - sorted.insert(tbest + 1, sorted.pop(l)) - - else: - sorted = list(basename_dict.keys()) # for merged directories - sorted.sort() - - self.rows = [basename_dict[key] for key in sorted] - def sort(self, labels_after=None, dirs_first=None, dirs_last=None, info_first=None):Keep only the second definition (lines 350-403).
33-56: Unusedanchorparameter in constructor.The
anchorparameter is documented and accepted but never used—self.anchoris always set toNoneinitially and later derived from the first appended pdsfile. Either use the parameter or remove it.Fix: Use the anchor parameter
def __init__(self, pdsfiles=None, parent=False, anchor=None, hidden=None): # ... self.parent_pdsf = parent - self.anchor = None + self.anchor = anchor self.rows = []viewmaster/pdsiterator.py (1)
78-90: Critical: Missingreturnafter handlingpdsf is None.When
pdsfisNone, the code sets initial attributes but then falls through to line 86+ which accessespdsf.NEIGHBORS, causing anAttributeError.Fix: Add return statement
if pdsf is None: self.neighbors = [] self.neighbor_index = 0 self.current_logical_path = None self.sign = 1 + self.neighbors_lc = [] + self.logger = logger + return - if isinstance(pdsf, pdsfile.Pds3File): + if isinstance(pdsf, pdsfile.Pds3File):viewmaster/viewmaster.py (6)
216-353: Don’tsys.exit(1)from library code; raise and let the caller decide.
get_holdings_path()callssys.exit(1)on failure, which will take down the WSGI worker/process if this path is used in production initialization. Also the comment mentionsPDS3_HOLDINGSbut the code usesPDS3_HOLDINGS_DIR. \Proposed patch (raise, and align message)
def get_holdings_paths(): @@ - pds3_holdings_dir = os.getenv('PDS3_HOLDINGS_DIR') # XXX PDS4 + pds3_holdings_dir = os.getenv('PDS3_HOLDINGS_DIR') # XXX PDS4 if pds3_holdings_dir is not None: return [pds3_holdings_dir] else: - raise IOError("'PDS3_HOLDINGS_DIR' environment variable not set") + raise OSError("'PDS3_HOLDINGS_DIR' environment variable not set") @@ def get_holdings_path(logger): @@ try: paths = get_holdings_paths() paths = validate_holdings_paths(paths, logger) - except Exception as e: + except Exception: logger.exception('Failed to get or validate holdings path') - sys.exit(1) + raise
541-654: Fixrow_pdsfileNameError for missing index-row navigation.In the
if query_pdsfile.is_index_row and not query_pdsfile.exists:block,row_pdsfileis referenced but never defined, so this path will crash. \Proposed patch
if query_pdsfile.is_index_row and not query_pdsfile.exists: @@ - row_pdsfile = row_pdsfile.copy() + row_pdsfile = query_copy.copy() row_pdsfile.nav_name = row_pdsfile.basename row_pdsfile.division = False row_pdsfile.terminated = False
1765-1801: Fix dict-iteration deletion + wrong return inget_query_params_from_dict().You delete keys while iterating
new_params(raisesRuntimeError) and then returnclean_query_params(params)instead of the normalized dict. \Proposed patch
def get_query_params_from_dict(params): @@ - # Remove unrecognized keys - new_params = params.copy() - for key in new_params: - if key not in defaults: - del new_params[key] + # Remove unrecognized keys + new_params = {k: v for k, v in params.items() if k in defaults} @@ for key in defaults: if key not in new_params: new_params[key] = defaults[key] - return clean_query_params(params) + return clean_query_params(new_params)
2347-2409: Declareglobal PAGE_CACHEinfill_page_cache()(assignment currently local-only).
PAGE_CACHE = get_page_cache(logger)creates a local variable and won’t update the module global. \Proposed patch
def fill_page_cache(logger): @@ + global PAGE_CACHE @@ - PAGE_CACHE = get_page_cache(logger) + PAGE_CACHE = get_page_cache(logger)
2142-2267: EnsureLOGGERandPAGE_CACHEare initialized on first request.
viewmaster()usesLOGGERandPAGE_CACHEdirectly (e.g.,LOGGER.info(),LOGGER.exception(),PAGE_CACHE[key]) without callingget_or_create_logger()or checking for initialization. When running under WSGI, the__main__block never executes, leaving these globals asNone. This crashes on the first request withAttributeError: 'NoneType' object has no attribute 'info'.Minimal patch (lazy initialization on first request)
def viewmaster(query_path): @@ - global LOGGER, PAGE_CACHE + global LOGGER, HOLDINGS_PATHS, PAGE_CACHE + + if LOGGER is None: + LOGGER = get_or_create_logger() + if HOLDINGS_PATHS is None: + HOLDINGS_PATHS = get_holdings_path(LOGGER) + if PAGE_CACHE is None: + PAGE_CACHE = get_page_cache(LOGGER)
1043-1077: Replace deprecatedcgi.escapewithhtml.escape(Python compatibility).
cgi.escapehas been removed in Python 3.11+ and will causeImportErrorat runtime. This must be replaced withhtml.escape, which has an identical signature including thequote=Trueparameter.Two usages require updating:
- Line 1074:
anchor_suffix = '#' + cgi.escape(params['selection'], quote=True)- Line 2007:
selection_escaped = cgi.escape(params['selection'], quote=True)Add
import htmlto the imports and replace both instances.Proposed patch
+import html @@ - anchor_suffix = '#' + cgi.escape(params['selection'], quote=True) + anchor_suffix = '#' + html.escape(params['selection'], quote=True) @@ - selection_escaped = cgi.escape(params['selection'], quote=True) + selection_escaped = html.escape(params['selection'], quote=True)
🤖 Fix all issues with AI agents
In @README.md:
- Around line 32-37: The README currently instructs creating /Library/WebServer
with sudo; change this to avoid requiring elevated privileges by updating the
docs and the app to use a user-writable default and an environment variable
override: replace the hardcoded /Library/WebServer recommendation with a
user-local default (e.g., ~/.viewmaster or a repo-local ./viewmaster_data) and
document an env var like VIEWMASTER_WEBROOT (or similar) that the application
reads to set the webroot; update README to show creating a user-writable
directory (no sudo) and how to set VIEWMASTER_WEBROOT, and change any code that
assumes /Library/WebServer to read the new env var and fall back to the
user-local default.
In @viewmaster/link.py:
- Around line 45-96: Extract the duplicated logger setup into a shared helper
(e.g., a new function in viewmaster/logging_utils.py) that encapsulates creating
a pdslogger.PdsLogger, adding the file handler with rotation, handling
OSError/IOError, and performing the two blankline/info startup call; then
replace Link's create_logger() to call that helper (passing the LOGNAME
replacement 'viewmaster'->'link', LOG_ROOT_PREFIX_+'link.log', limits and pid
flags) and keep get_or_create_logger() as a thin cached wrapper that delegates
to the shared helper; ensure unique symbols to update are create_logger,
get_or_create_logger in viewmaster/link.py and the new exported helper name
(e.g., setup_pds_logger) in the shared module.
- Around line 67-68: The except clause currently catches the same exception
twice ("except (OSError, IOError) as e") — simplify it by removing the redundant
alias and catch only OSError; update the exception handler around the
logger.warning call (the except block that references info_logfile and
logger.warning) to use "except OSError as e" so the code is clearer and avoids
duplicate exception types.
In @viewmaster/pdsgroup.py:
- Around line 538-544: Replace the explicit list comprehension in
iterator_for_all with a direct list conversion for clarity and performance:
change the return in PdsGroup.iterator_for_all from using "[r for r in
self.rows]" to using "list(self.rows)" so it simply returns list(self.rows).
In @viewmaster/pdsiterator.py:
- Around line 311-317: In PdsFileIterator.copy() the for-loop trying
Pds3File.from_logical_path and Pds4File.from_logical_path can leave pdsf
undefined if both raise ValueError; add an else: on the for-loop that raises a
clear ValueError (or RuntimeError) mentioning the invalid
self.current_logical_path so the method fails fast, then proceed to construct
PdsFileIterator(pdsf, sign=sign1, ...) as before; reference the loop over
pdsfile.Pds3File and pdsfile.Pds4File and the variable pdsf when adding this
guard.
- Around line 155-161: The loop over pdsfile.Pds3File and pdsfile.Pds4File can
exit without defining pdsf if both from_logical_path calls raise ValueError;
update the loop to use a for/else pattern like next_cousin: try
from_logical_path on each class_name, break on success, and add an else that
raises a clear ValueError (or logs and raises) when neither succeeds so
PdsDirIterator(pdsf, ...) never receives an unbound pdsf; reference the symbols
pdsfile.Pds3File, pdsfile.Pds4File, from_logical_path, and PdsDirIterator when
making the change.
In @viewmaster/viewmaster_config.py:
- Around line 38-39: Replace the fragile filename check for VIEWMASTER_TESTING
with an explicit environment-variable-based toggle: check
os.environ.get('VIEWMASTER_TESTING') (e.g. equals '1' or a truthy value) to
determine testing mode, falling back to the existing
sys.argv[0].endswith('viewmaster.py') only if the env var is not set; update the
VIEWMASTER_TESTING assignment and add an import for os if missing so the new
env-var check is used by the module.
In @viewmaster/viewmaster.py:
- Around line 2106-2125: The return_holdings_local route builds file_path by
concatenating HOLDINGS_PATHS[0] and query_path which allows path traversal;
replace this with a safe serve-from-directory approach: use Flask's
send_from_directory(HOLDINGS_PATHS[0], query_path, mimetype=...) or explicitly
resolve and validate the realpath of the joined path (os.path.realpath) and
ensure it startswith the holdings root before calling send_file; update
return_holdings_local to perform this safe-join/validation and
return_from_directory/send_file only after confirming the target is inside
HOLDINGS_PATHS[0].
- Around line 355-400: get_page_cache currently reassigns
VIEWMASTER_MEMCACHE_PORT inside the function causing an UnboundLocalError;
instead, capture the global value into a local variable (e.g., memcache_port =
VIEWMASTER_MEMCACHE_PORT) at the start of get_page_cache, use memcache_port for
the conditional checks and when creating pdscache.MemcachedCache, and set
memcache_port = 0 on failure (inside the except) rather than mutating the
global; leave PAGE_CACHE handling as-is and do not add a global declaration for
VIEWMASTER_MEMCACHE_PORT.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
README.mdviewmaster/link.pyviewmaster/pdsgroup.pyviewmaster/pdsiterator.pyviewmaster/viewmaster.pyviewmaster/viewmaster_config.py
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:16.187Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:21-22
Timestamp: 2025-12-18T21:42:57.270Z
Learning: In viewmaster, use `pdsfile.Pds3File` explicitly in code implementation instead of the generic `pdsfile.PdsFile` base class in pdsgrouptable.py and pdsiterator.py. Docstrings should reference `Pds3File`, with `Pds4File` to be added later when PDS4 is fully supported.
📚 Learning: 2026-01-12T21:03:16.187Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:16.187Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
Applied to files:
README.mdviewmaster/viewmaster_config.pyviewmaster/pdsiterator.pyviewmaster/viewmaster.py
📚 Learning: 2025-12-12T23:26:32.767Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: docs/Makefile:11-20
Timestamp: 2025-12-12T23:26:32.767Z
Learning: In the rms-viewmaster repository, the `docs/Makefile` is auto-generated by Sphinx and should not be modified, even if linters flag missing standard phony targets.
Applied to files:
README.mdviewmaster/viewmaster.py
📚 Learning: 2026-01-12T22:07:33.958Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/viewmaster.py:2496-2501
Timestamp: 2026-01-12T22:07:33.958Z
Learning: In the rms-viewmaster repository, the `if __name__ == "__main__":` block in `viewmaster/viewmaster.py` with `debug=True` is intentionally kept for development purposes only. This code path is not executed in production, which uses a different deployment method (e.g., WSGI server).
Applied to files:
README.mdviewmaster/viewmaster_config.py
📚 Learning: 2025-12-18T21:42:57.270Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:21-22
Timestamp: 2025-12-18T21:42:57.270Z
Learning: In viewmaster, use `pdsfile.Pds3File` explicitly in code implementation instead of the generic `pdsfile.PdsFile` base class in pdsgrouptable.py and pdsiterator.py. Docstrings should reference `Pds3File`, with `Pds4File` to be added later when PDS4 is fully supported.
Applied to files:
viewmaster/pdsgroup.pyviewmaster/viewmaster.py
📚 Learning: 2025-12-18T21:42:45.915Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:21-22
Timestamp: 2025-12-18T21:42:45.915Z
Learning: In viewmaster/pdsgrouptable.py and viewmaster/pdsiterator.py, implement and reference Pds3File explicitly instead of using the generic PdsFile base class. Update docstrings to reference Pds3File, with Pds4File to be introduced later when PDS4 is fully supported. This guidance should be reflected in imports, instantiations, and type hints to improve specificity and future maintainability.
Applied to files:
viewmaster/pdsiterator.py
📚 Learning: 2026-01-12T21:13:15.917Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:460-467
Timestamp: 2026-01-12T21:13:15.917Z
Learning: In viewmaster's pdsgrouptable.py `tables_from_pdsfiles` and similar methods, when a path string cannot be resolved as either Pds3File or Pds4File, the program should be allowed to crash rather than silently skipping the file. This fail-fast approach is preferred for invalid path inputs.
Applied to files:
viewmaster/pdsiterator.pyviewmaster/viewmaster.py
📚 Learning: 2026-01-12T22:07:33.958Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/viewmaster.py:2496-2501
Timestamp: 2026-01-12T22:07:33.958Z
Learning: In viewmaster/viewmaster.py, the if __name__ == '__main__': block that sets debug=True is for development only and should not run in production. Ensure production deployment uses the WSGI server path and that any debug execution is removed or guarded (e.g., do not enable debug mode in the main entry point for production).
Applied to files:
viewmaster/viewmaster.py
🧬 Code graph analysis (4)
viewmaster/pdsgroup.py (1)
viewmaster/pdsgrouptable.py (3)
iterator(120-126)iterator_for_all(128-134)iterator_for_hidden(136-142)
viewmaster/link.py (1)
viewmaster/viewmaster.py (2)
create_logger(81-121)get_or_create_logger(124-136)
viewmaster/pdsiterator.py (2)
viewmaster/pdsgroup.py (2)
isdir(135-144)parent_logical_path(122-132)viewmaster/pdsgrouptable.py (2)
filter(390-401)parent_logical_path(82-91)
viewmaster/viewmaster.py (3)
viewmaster/pdsgroup.py (3)
PdsGroup(13-598)isdir(135-144)copy(100-119)viewmaster/link.py (2)
create_logger(45-82)get_or_create_logger(84-96)viewmaster/pdsiterator.py (4)
PdsDirIterator(50-231)copy(139-163)copy(303-321)copy(500-510)
🪛 ast-grep (0.40.4)
viewmaster/viewmaster.py
[warning] 2517-2517: Detected Flask app with debug=True. Do not deploy to production with this flag enabled as it will leak sensitive information. Instead, consider using Flask configuration variables or setting 'debug' using system environment variables.
Context: app.run(host='0.0.0.0', port=8080, debug=True)
Note: [CWE-489] Active Debug Code. [REFERENCES]
- https://labs.detectify.com/2015/10/02/how-patreon-got-hacked-publicly-exposed-werkzeug-debugger/
(debug-enabled-python)
🪛 LanguageTool
README.md
[style] ~5-~5: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1401 characters long)
Context: ...-----|-------------|----------------| |
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
README.md
3-3: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
42-42: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 Ruff (0.14.10)
viewmaster/pdsgroup.py
1-1: The file is executable but no shebang is present
(EXE002)
33-33: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
33-33: Boolean default positional argument in function definition
(FBT002)
33-33: Unused method argument: anchor
(ARG002)
74-74: Missing return type annotation for special method __len__
Add return type annotation: int
(ANN204)
83-83: Missing return type annotation for special method __repr__
Add return type annotation: str
(ANN204)
544-544: Unnecessary list comprehension (rewrite using list())
Rewrite using list()
(C416)
viewmaster/viewmaster_config.py
1-1: The file is executable but no shebang is present
(EXE002)
72-72: Probable insecure usage of temporary file or directory: "/var/tmp/memcached.socket"
(S108)
73-73: Probable insecure usage of temporary file or directory: "/var/tmp/memcached.socket"
(S108)
viewmaster/link.py
1-1: The file is executable but no shebang is present
(EXE002)
40-40: from .viewmaster_config import * used; unable to detect undefined names
(F403)
56-56: LOGNAME may be undefined, or defined from star imports
(F405)
60-60: LOG_ROOT_PREFIX_ may be undefined, or defined from star imports
(F405)
67-67: Replace aliased errors with OSError
Replace with builtin OSError
(UP024)
68-68: Logging statement uses f-string
(G004)
142-142: VIEWMASTER_PREFIX_ may be undefined, or defined from star imports
(F405)
146-146: DOCUMENT_ROOT_ may be undefined, or defined from star imports
(F405)
viewmaster/pdsiterator.py
1-1: The file is executable but no shebang is present
(EXE002)
65-65: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
188-188: Missing return type annotation for special method __next__
(ANN204)
259-259: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
323-323: Missing return type annotation for private function _filter_names
(ANN202)
366-366: Missing return type annotation for special method __next__
(ANN204)
426-430: Avoid specifying long messages outside the exception class
(TRY003)
468-468: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
535-535: Missing return type annotation for special method __next__
(ANN204)
viewmaster/viewmaster.py
1-1: The file is executable but no shebang is present
(EXE002)
73-73: from .viewmaster_config import * used; unable to detect undefined names
(F403)
74-74: USE_SHELVES_ONLY may be undefined, or defined from star imports
(F405)
75-75: Boolean positional value in function call
(FBT003)
93-93: LOGNAME may be undefined, or defined from star imports
(F405)
95-95: LOGNAME may be undefined, or defined from star imports
(F405)
98-98: LOG_ROOT_PREFIX_ may be undefined, or defined from star imports
(F405)
108-108: LOG_ROOT_PREFIX_ may be undefined, or defined from star imports
(F405)
232-232: Replace aliased errors with OSError
Replace IOError with builtin OSError
(UP024)
232-232: Avoid specifying long messages outside the exception class
(TRY003)
345-345: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
373-373: PAGE_CACHING may be undefined, or defined from star imports
(F405)
374-374: VIEWMASTER_MEMCACHE_PORT may be undefined, or defined from star imports
(F405)
376-377: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
376-377: Logging statement uses %
(G002)
377-377: VIEWMASTER_MEMCACHE_PORT may be undefined, or defined from star imports
(F405)
378-378: VIEWMASTER_MEMCACHE_PORT may be undefined, or defined from star imports
(F405)
384-385: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
384-385: Logging statement uses %
(G002)
385-385: VIEWMASTER_MEMCACHE_PORT may be undefined, or defined from star imports
(F405)
407-407: Boolean default positional argument in function definition
(FBT002)
1353-1353: datetime.datetime.now() called without a tz argument
(DTZ005)
1355-1355: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1355-1355: Logging statement uses %
(G002)
2262-2262: Unnecessary pass statement
Remove unnecessary pass
(PIE790)
🔇 Additional comments (4)
viewmaster/pdsiterator.py (1)
417-430: Good error handling innext_cousin.The
for/elsepattern correctly raises a descriptiveValueErrorwhen neitherPds3FilenorPds4Filecan create the parent. This pattern should be applied consistently to thecopy()methods as noted above.viewmaster/link.py (1)
99-154: LGTM: Route handler implementation.The redirect logic correctly distinguishes directories from files using extension detection and handles the 404 case with proper logging. The per-request logger pattern aligns with the broader refactoring goals.
README.md (1)
57-58: Update documentation link after merge.The TODO comment indicates the readthedocs link needs updating once the branch is merged to main. Consider using a placeholder or the main branch link if the docs are already published.
Would you like me to open an issue to track updating this link post-merge?
⛔ Skipped due to learnings
Learnt from: juzen2003 Repo: SETI/rms-viewmaster PR: 14 File: README.md:16-18 Timestamp: 2026-01-12T21:03:16.187Z Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).Learnt from: juzen2003 Repo: SETI/rms-viewmaster PR: 14 File: docs/Makefile:11-20 Timestamp: 2025-12-12T23:26:32.767Z Learning: In the rms-viewmaster repository, the `docs/Makefile` is auto-generated by Sphinx and should not be modified, even if linters flag missing standard phony targets.viewmaster/viewmaster.py (1)
2509-2519: No action required—this is development-only code.The
__main__block is intentionally preserved for local development and is not executed in production, which uses a WSGI server setup (seewsgi_init.py). Thedebug=Trueand0.0.0.0binding pose no production risk in this context.
| 4. Create and set ownership for `/Library/WebServer` to avoid permission issues: | ||
|
|
||
| ```bash | ||
| sudo mkdir /Library/WebServer | ||
| sudo chown -R user /Library/WebServer # Replace "user" with your own username | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid requiring sudo for local development setup.
The instructions require sudo to create /Library/WebServer, which was flagged in PR discussions as indicating a configuration/permission problem. Consider either:
- Using a user-writable directory (e.g.,
~/.viewmaster/or a directory within the repo) - Making this path configurable via an environment variable
This would allow developers to run viewmaster without elevated privileges.
🤖 Prompt for AI Agents
In @README.md around lines 32 - 37, The README currently instructs creating
/Library/WebServer with sudo; change this to avoid requiring elevated privileges
by updating the docs and the app to use a user-writable default and an
environment variable override: replace the hardcoded /Library/WebServer
recommendation with a user-local default (e.g., ~/.viewmaster or a repo-local
./viewmaster_data) and document an env var like VIEWMASTER_WEBROOT (or similar)
that the application reads to set the webroot; update README to show creating a
user-writable directory (no sudo) and how to set VIEWMASTER_WEBROOT, and change
any code that assumes /Library/WebServer to read the new env var and fall back
to the user-local default.
There was a problem hiding this comment.
Leave as-is, the default WEBSITE_ROOT_, DOCUMENT_ROOT_, and LOG_ROOT_PREFIX_ are all under /Library/WebServer. We don't plan to change it at this point.
There was a problem hiding this comment.
@juzen2003, understood! I've noted your preference to keep the /Library/WebServer default configuration.
🧠 Learnings used
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/viewmaster.py:2496-2501
Timestamp: 2026-01-12T22:07:39.297Z
Learning: In the rms-viewmaster repository, the `if __name__ == "__main__":` block in `viewmaster/viewmaster.py` with `debug=True` is intentionally kept for development purposes only. This code path is not executed in production, which uses a different deployment method (e.g., WSGI server).
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:22.730Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
There was a problem hiding this comment.
This is not quite true. These are not the directories to use under Linux. We use /var/www. But I expect this documentation will end up being mostly wrong anyway by the time we finish.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| def create_logger(): | ||
| """Create and configure a logger for the Link service. | ||
| # DEBUG_LOG_FILE = LOG_ROOT_PREFIX_ + 'link_debug.log' | ||
| # debug_logfile = os.path.abspath(DEBUG_LOG_FILE) | ||
| # debug_handler = pdslogger.file_handler(debug_logfile, level=logging.DEBUG, | ||
| # rotation='midnight') | ||
| # LOGGER.add_handler(debug_handler) | ||
| Sets up a PdsLogger instance with INFO level logging to a file with | ||
| midnight rotation. The logger name is derived from the Viewmaster logger | ||
| name by replacing 'viewmaster' with 'link'. | ||
| ################################################################################ | ||
| Returns: | ||
| pdslogger.PdsLogger: Configured logger instance. | ||
| """ | ||
|
|
||
| link_logger_name = LOGNAME.replace('viewmaster', 'link') | ||
| logger = pdslogger.PdsLogger(link_logger_name, limits={'info': -1, 'normal': -1}, | ||
| pid=True) | ||
|
|
||
| LOG_FILE = LOG_ROOT_PREFIX_ + 'link.log' | ||
| info_logfile = os.path.abspath(LOG_FILE) | ||
|
|
||
| try: | ||
| info_handler = pdslogger.file_handler(info_logfile, level=logging.INFO, | ||
| rotation='midnight') | ||
| logger.add_handler(info_handler) | ||
| except (OSError, IOError) as e: | ||
| logger.warning(f'Could not open log file {info_logfile}: {e}') | ||
|
|
||
| # DEBUG_LOG_FILE = LOG_ROOT_PREFIX_ + 'link_debug.log' | ||
| # debug_logfile = os.path.abspath(DEBUG_LOG_FILE) | ||
| # debug_handler = pdslogger.file_handler(debug_logfile, level=logging.DEBUG, | ||
| # rotation='midnight') | ||
| # logger.add_handler(debug_handler) | ||
|
|
||
| ################################################################################ | ||
|
|
||
| logger.blankline() | ||
| logger.blankline() | ||
| logger.info('Starting Link', info_logfile) | ||
|
|
||
| return logger | ||
|
|
||
| def get_or_create_logger(): | ||
| """Get the cached logger instance, creating it if necessary. | ||
| This ensures the logger and its handlers are only created once, preventing | ||
| duplicate handlers from being added on subsequent requests. | ||
| Returns: | ||
| pdslogger.PdsLogger: The cached logger instance. | ||
| """ | ||
| global LOGGER | ||
| if LOGGER is None: | ||
| LOGGER = create_logger() | ||
| return LOGGER |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared logger utilities to reduce duplication.
The create_logger() and get_or_create_logger() functions here closely mirror those in viewmaster/viewmaster.py (lines 80-135). Consider extracting common logger setup logic to a shared module (e.g., viewmaster/logging_utils.py) to reduce duplication and ensure consistent configuration.
🧰 Tools
🪛 Ruff (0.14.10)
56-56: LOGNAME may be undefined, or defined from star imports
(F405)
60-60: LOG_ROOT_PREFIX_ may be undefined, or defined from star imports
(F405)
67-67: Replace aliased errors with OSError
Replace with builtin OSError
(UP024)
68-68: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In @viewmaster/link.py around lines 45 - 96, Extract the duplicated logger setup
into a shared helper (e.g., a new function in viewmaster/logging_utils.py) that
encapsulates creating a pdslogger.PdsLogger, adding the file handler with
rotation, handling OSError/IOError, and performing the two blankline/info
startup call; then replace Link's create_logger() to call that helper (passing
the LOGNAME replacement 'viewmaster'->'link', LOG_ROOT_PREFIX_+'link.log',
limits and pid flags) and keep get_or_create_logger() as a thin cached wrapper
that delegates to the shared helper; ensure unique symbols to update are
create_logger, get_or_create_logger in viewmaster/link.py and the new exported
helper name (e.g., setup_pds_logger) in the shared module.
There was a problem hiding this comment.
@juzen2003, understood! I'll leave the logger setup as-is in both files.
🧠 Learnings used
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:22.730Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
| # Test for a command line run, python viewmaster/viewmaster.py | ||
| VIEWMASTER_TESTING = sys.argv[0].endswith('viewmaster.py') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for VIEWMASTER_TESTING usage
rg "VIEWMASTER_TESTING" --type py -B 2 -A 2Repository: SETI/rms-viewmaster
Length of output: 1293
🏁 Script executed:
# Check for any scripts ending in viewmaster.py
fd -e py | grep viewmasterRepository: SETI/rms-viewmaster
Length of output: 238
🏁 Script executed:
# Examine the viewmaster_config.py file to understand context
cat -n viewmaster/viewmaster_config.py | head -100Repository: SETI/rms-viewmaster
Length of output: 4081
🏁 Script executed:
# Check the main viewmaster.py script to understand deployment
cat -n viewmaster/viewmaster.py | head -50Repository: SETI/rms-viewmaster
Length of output: 1715
Testing mode detection could unintentionally trigger for any script ending with viewmaster.py.
Using sys.argv[0].endswith('viewmaster.py') could match unintended scripts like test_viewmaster.py or my_viewmaster.py, though this is unlikely given the current codebase and production deployment via WSGI. If strengthening this check, consider an environment variable like VIEWMASTER_TESTING=1 for explicit control, which is simpler than path pattern matching.
🤖 Prompt for AI Agents
In @viewmaster/viewmaster_config.py around lines 38 - 39, Replace the fragile
filename check for VIEWMASTER_TESTING with an explicit
environment-variable-based toggle: check os.environ.get('VIEWMASTER_TESTING')
(e.g. equals '1' or a truthy value) to determine testing mode, falling back to
the existing sys.argv[0].endswith('viewmaster.py') only if the env var is not
set; update the VIEWMASTER_TESTING assignment and add an import for os if
missing so the new env-var check is used by the module.
There was a problem hiding this comment.
@juzen2003, understood. I'll keep the current implementation as-is.
🧠 Learnings used
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/viewmaster.py:2496-2501
Timestamp: 2026-01-12T22:07:39.297Z
Learning: In the rms-viewmaster repository, the `if __name__ == "__main__":` block in `viewmaster/viewmaster.py` with `debug=True` is intentionally kept for development purposes only. This code path is not executed in production, which uses a different deployment method (e.g., WSGI server).
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:22.730Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
Updated the import statements in link.py to use a more structured format, enhancing readability and maintainability of the code. This change groups related configuration variables together, making it easier to understand dependencies at a glance.
Updated the exception handling in create_logger to remove the IOError catch, simplifying the error logging process. This change enhances the clarity of log messages when failing to open the log file.
Updated the member retrieval method to use the built-in list constructor instead of a list comprehension. This change enhances code clarity and maintains consistency in the way members are returned.
Refactored the create_logger function to check for existing handlers before adding new ones, preventing duplicates. Added exception handling for file handler creation to log warnings when log files cannot be opened, improving the robustness and clarity of the logging process.
…larity Updated the Memcache connection logic to use a local variable for the port, enhancing readability and simplifying error handling. This change ensures that the fallback to DictionaryCache is clearer and more robust, improving the overall caching mechanism.
…nality Updated the return_holdings_local function to utilize send_from_directory for serving files, improving the handling of MIME types and simplifying the code structure. This change enhances the clarity of file serving logic and ensures better adherence to Flask's best practices.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
viewmaster/pdsgroup.py (1)
296-403: Duplicatesort()method definition.The
sort()method is defined twice with identical signatures (lines 296-348 and lines 350-403). In Python, the second definition completely shadows the first, making the first implementation dead code. The implementations appear nearly identical—remove one of them.🐛 Proposed fix: remove duplicate definition
- def sort(self, labels_after=None, dirs_first=None, dirs_last=None, - info_first=None): - """Sort the rows of this group. - - Parameters: - labels_after (bool|None): Place labels after their targets. - dirs_first (bool|None): Sort directories before files. - dirs_last (bool|None): Sort directories after files. - info_first (bool|None): Prioritize info files. - - Returns: - None - """ - - basename_dict = {pdsf.basename:pdsf for pdsf in self.rows} - - if self.parent_pdsf: - sorted = self.parent_pdsf.sort_basenames(list(basename_dict.keys()), - labels_after=labels_after, - dirs_first=dirs_first, - dirs_last=dirs_last, - info_first=info_first) - - # If there are multiple labels in the group, make sure each label - # is adjacent to any data files of the same name. This can comes up - # with Juno JIRAM, at least. - labels = [basename for basename in sorted - if self.parent_pdsf.basename_is_label(basename)] - if len(labels) > 1: - for label in labels: - l = sorted.index(label) # index of label - pattern = os.path.splitext(label)[0] + '.' - tlist = [t for t in range(len(sorted)) # indices of targets - if sorted[t].startswith(pattern)] - tlist.remove(l) - - # If the label is not adjacent, but could be... - if tlist and (l+1) not in tlist and (l-1) not in tlist: - # Locate the nearest target with a matching basename - diffs = [(abs(t-l),t) for t in tlist] - tbest = min(diffs)[1] - - # Relocate the label to an adjacent location - if l < tbest: - sorted.insert(tbest - 1, sorted.pop(l)) - else: - sorted.insert(tbest + 1, sorted.pop(l)) - - else: - sorted = list(basename_dict.keys()) # for merged directories - sorted.sort() - - self.rows = [basename_dict[key] for key in sorted] - def sort(self, labels_after=None, dirs_first=None, dirs_last=None, info_first=None): """Sort member files by parent-defined rules with tweaks for labels.viewmaster/viewmaster.py (1)
1783-1818: Bug:get_query_params_from_dictreturns cleaned version of wrong variable.Line 1818 returns
clean_query_params(params)but should returnclean_query_params(new_params). The function buildsnew_paramswith defaults applied, but then cleans and returns the originalparamsdict, ignoring all the work done.🐛 Proposed fix
# Fill in default values for missing keys for key in defaults: if key not in new_params: new_params[key] = defaults[key] - return clean_query_params(params) + return clean_query_params(new_params)
🤖 Fix all issues with AI agents
In `@viewmaster/pdsgroup.py`:
- Line 33: The __init__ of PdsGroup currently accepts an unused parameter
anchor; either remove the parameter or assign it to the instance. To implement
the intended behavior, set self.anchor = anchor (or a default if None) in
PdsGroup.__init__, and ensure any existing logic that derives anchor from the
first appended PdsFile (e.g., code in append/appendPdsFile or methods
referencing self.anchor) respects an already-set self.anchor (i.e., do not
overwrite it if anchor was provided). Update any docstring/comments to reflect
that anchor can be provided by the constructor.
In `@viewmaster/viewmaster.py`:
- Around line 2274-2280: Remove the redundant `pass` inside the except block
that follows the `LOGGER.warning('Fancy index unavailable; abort(404)')` call;
locate the try/except surrounding the redirect (the block that logs "Fancy index
unavailable; abort(404)") and simply delete the `pass` line so the except block
contains only the warning call and control flow continues to the subsequent
`LOGGER.warning('ABORT 404')`.
- Around line 80-86: Replace the wildcard import from viewmaster_config with
explicit named imports: import USE_SHELVES_ONLY and every other config symbol
referenced in this module (e.g., LOGNAME, LOG_ROOT_PREFIX_, VIEWMASTER_PREFIX_,
DOCUMENT_ROOT_, WEBSITE_HTTP_HOME, LOCALHOST_, VIEWMASTER_MEMCACHE_PORT,
PDSFILE_MEMCACHE_PORT, PAGE_CACHING, EXTRA_LOCAL_IP_ADDRESS_A_B_C, etc.) so
static analysis no longer reports F403/F405; keep the
Pds3File.use_shelves_only(True) call but ensure Pds3File is imported or
available and update the top-of-file imports to list each required symbol
explicitly rather than using from .viewmaster_config import *.
- Around line 235-249: The function get_holdings_paths currently raises IOError;
replace that with OSError for consistency with the rest of the file (e.g., other
OSError uses) and update the docstring "Raises" section to list OSError instead
of IOError; specifically change the raise statement in get_holdings_paths to
raise OSError("'PDS3_HOLDINGS_DIR' environment variable not set") and adjust the
Returns/Raises docstring accordingly.
- Around line 359-369: The except block captures an exception as variable `e`
but never uses it; either remove the unused `e` (change to `except Exception:`)
or include the exception in the log (e.g., `logger.exception('Failed to get or
validate holdings path: %s', e)` or rely on `logger.exception`'s traceback by
passing `exc_info=True`), updating the except clause around
`get_holdings_paths`/`validate_holdings_paths` accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
viewmaster/link.pyviewmaster/pdsgroup.pyviewmaster/viewmaster.py
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:22.730Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
📚 Learning: 2026-01-12T21:03:22.730Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:22.730Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
Applied to files:
viewmaster/link.pyviewmaster/viewmaster.py
📚 Learning: 2025-12-18T21:42:57.270Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:21-22
Timestamp: 2025-12-18T21:42:57.270Z
Learning: In viewmaster, use `pdsfile.Pds3File` explicitly in code implementation instead of the generic `pdsfile.PdsFile` base class in pdsgrouptable.py and pdsiterator.py. Docstrings should reference `Pds3File`, with `Pds4File` to be added later when PDS4 is fully supported.
Applied to files:
viewmaster/pdsgroup.pyviewmaster/viewmaster.py
📚 Learning: 2026-01-12T22:07:33.958Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/viewmaster.py:2496-2501
Timestamp: 2026-01-12T22:07:33.958Z
Learning: In viewmaster/viewmaster.py, the if __name__ == '__main__': block that sets debug=True is for development only and should not run in production. Ensure production deployment uses the WSGI server path and that any debug execution is removed or guarded (e.g., do not enable debug mode in the main entry point for production).
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2025-12-12T23:26:32.767Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: docs/Makefile:11-20
Timestamp: 2025-12-12T23:26:32.767Z
Learning: In the rms-viewmaster repository, the `docs/Makefile` is auto-generated by Sphinx and should not be modified, even if linters flag missing standard phony targets.
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2026-01-12T21:13:24.268Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:460-467
Timestamp: 2026-01-12T21:13:24.268Z
Learning: In viewmaster's pdsgrouptable.py `tables_from_pdsfiles` and similar methods, when a path string cannot be resolved as either Pds3File or Pds4File, the program should be allowed to crash rather than silently skipping the file. This fail-fast approach is preferred for invalid path inputs.
Applied to files:
viewmaster/viewmaster.py
🧬 Code graph analysis (2)
viewmaster/link.py (1)
viewmaster/viewmaster.py (2)
create_logger(88-138)get_or_create_logger(141-153)
viewmaster/pdsgroup.py (1)
viewmaster/pdsgrouptable.py (3)
iterator(120-126)iterator_for_all(128-134)iterator_for_hidden(136-142)
🪛 ast-grep (0.40.5)
viewmaster/viewmaster.py
[warning] 2532-2532: Detected Flask app with debug=True. Do not deploy to production with this flag enabled as it will leak sensitive information. Instead, consider using Flask configuration variables or setting 'debug' using system environment variables.
Context: app.run(host='0.0.0.0', port=8080, debug=True)
Note: [CWE-489] Active Debug Code. [REFERENCES]
- https://labs.detectify.com/2015/10/02/how-patreon-got-hacked-publicly-exposed-werkzeug-debugger/
(debug-enabled-python)
🪛 Ruff (0.14.11)
viewmaster/link.py
1-1: The file is executable but no shebang is present
(EXE002)
74-74: Logging statement uses f-string
(G004)
viewmaster/pdsgroup.py
1-1: The file is executable but no shebang is present
(EXE002)
33-33: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
33-33: Boolean default positional argument in function definition
(FBT002)
33-33: Unused method argument: anchor
(ARG002)
74-74: Missing return type annotation for special method __len__
Add return type annotation: int
(ANN204)
83-83: Missing return type annotation for special method __repr__
Add return type annotation: str
(ANN204)
viewmaster/viewmaster.py
1-1: The file is executable but no shebang is present
(EXE002)
80-80: from .viewmaster_config import * used; unable to detect undefined names
(F403)
81-81: USE_SHELVES_ONLY may be undefined, or defined from star imports
(F405)
82-82: Boolean positional value in function call
(FBT003)
100-100: LOGNAME may be undefined, or defined from star imports
(F405)
102-102: LOGNAME may be undefined, or defined from star imports
(F405)
105-105: LOG_ROOT_PREFIX_ may be undefined, or defined from star imports
(F405)
119-119: Logging statement uses f-string
(G004)
122-122: LOG_ROOT_PREFIX_ may be undefined, or defined from star imports
(F405)
130-130: Logging statement uses f-string
(G004)
249-249: Replace aliased errors with OSError
Replace IOError with builtin OSError
(UP024)
249-249: Avoid specifying long messages outside the exception class
(TRY003)
362-362: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
390-390: PAGE_CACHING may be undefined, or defined from star imports
(F405)
391-391: VIEWMASTER_MEMCACHE_PORT may be undefined, or defined from star imports
(F405)
394-395: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
394-395: Logging statement uses %
(G002)
402-403: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
402-403: Logging statement uses %
(G002)
425-425: Boolean default positional argument in function definition
(FBT002)
1371-1371: datetime.datetime.now() called without a tz argument
(DTZ005)
1373-1373: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1373-1373: Logging statement uses %
(G002)
2277-2277: Unnecessary pass statement
Remove unnecessary pass
(PIE790)
🔇 Additional comments (22)
viewmaster/link.py (4)
1-14: LGTM!The module docstring provides a clear overview of the Link redirect service functionality, including directory detection logic and redirect behavior.
40-46: LGTM!The explicit imports from
viewmaster_configare well-structured, replacing the previous star import. This improves code clarity and satisfies static analysis requirements.
48-102: LGTM!The logger creation and caching pattern is well-implemented:
create_logger()properly handles file handler errors with a warning instead of crashingget_or_create_logger()provides proper singleton caching to avoid duplicate handlers- The pattern mirrors
viewmaster.pyappropriately for this module's needs
105-160: LGTM!The route handler correctly:
- Uses
get_or_create_logger()to avoid per-request logger creation- Implements directory detection via extension/digit heuristics
- Uses glob patterns to locate files across holdings symlinks
- Logs actions appropriately for debugging
viewmaster/pdsgroup.py (5)
1-31: LGTM!The module docstring and class documentation are comprehensive, clearly explaining the purpose of PdsGroup and its attributes.
33-72: LGTM!The
__init__method correctly usesNoneas defaults for mutable arguments (pdsfilesandhidden), avoiding the shared-state bug that was previously flagged. The initialization logic properly handles all input types.
74-119: LGTM!The
__len__,__repr__, andcopy()implementations are correct and well-documented.
405-552: LGTM!The append/remove/hide/unhide methods and iterator helpers are well-implemented with proper validation, duplicate detection, and cache invalidation.
558-598: LGTM!The
group_childrenstatic method provides a clean interface for grouping a directory's children by anchor.viewmaster/viewmaster.py (13)
1-18: LGTM!The module docstring provides a clear overview of the Viewmaster Flask application, including environment variable and configuration requirements.
88-153: LGTM!The logger creation and caching implementation is well-structured:
create_logger()guards against duplicate handlers withhas_handlerscheck- Error handling wraps file handler creation to prevent crashes
get_or_create_logger()provides proper singleton pattern- Both INFO and DEBUG log files are handled consistently
372-417: LGTM!The
get_page_cache()function properly:
- Uses a local
memcache_portvariable to avoid the UnboundLocalError- Falls back gracefully to DictionaryCache on Memcache failure
- Logs the cache backend being used
425-447: LGTM!The
initialize_caches()function correctly initializes holdings and page caches, with optional reset capability.
559-671: LGTM!The
get_prev_next_navigation()function is well-implemented with proper iterator handling, navigation constraints, and edge case handling for virtual directories and index rows.
981-1057: LGTM!The
get_directory_page()function properly assembles page data including groups, associations, documents, navigation, and info content.
1061-1275: LGTM!The
directory_page_html()function handles filtering, pagination, row ranges, and link generation correctly, with proper redirect handling when parameters need adjustment.
1281-1447: LGTM!The
get_product_page_info()function handles complex product page assembly including siblings, info files, navigation, and index row special cases.
1451-1659: LGTM!The
product_page_html()function properly renders product views with filtering, multipage support, and special handling for index rows.
2124-2139: LGTM!The
return_holdings_local()function correctly usessend_from_directory()which provides path traversal protection, addressing the previously flagged security concern.
2157-2284: LGTM!The main
viewmaster()route handler properly:
- Uses cached pages when available
- Handles redirects for path normalization and parameter adjustment
- Logs requests with timing information
- Handles errors gracefully with fallback to fancy index or 404 page
2362-2423: LGTM!The
fill_page_cache()function properly walks the holdings structure to warm the cache. The logger parameter is correctly threaded through.
2524-2533: LGTM!The
__main__block correctly initializes the logger, holdings paths, and page cache before starting the Flask development server. Per previous discussion,debug=Trueis acceptable here since this block is only executed during development.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Modified the anchor attribute in the PdsGroup class to accept an argument during initialization, enhancing the clarity of its purpose and ensuring it is set correctly upon object creation. This change improves the overall readability and maintainability of the code.
Updated the import statements to use a structured format, grouping related configuration variables together. This change enhances readability and maintainability of the code, making dependencies clearer at a glance.
Updated the exception handling in multiple functions to raise OSError instead of IOError when the PDS3_HOLDINGS_DIR environment variable is not set or when no valid holdings paths remain. This change improves consistency in error handling and aligns with Python's standard exceptions for operating system-related errors.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
viewmaster/pdsgroup.py (2)
296-403: Critical: Duplicatesortmethod definition.The
sortmethod is defined twice (lines 296-348 and lines 350-403). The second definition completely shadows the first, making the first implementation dead code. Both implementations appear identical, so this is likely a copy-paste error.🐛 Proposed fix: Remove the duplicate definition
- def sort(self, labels_after=None, dirs_first=None, dirs_last=None, - info_first=None): - """Sort the rows of this group. - - Parameters: - labels_after (bool|None): Place labels after their targets. - dirs_first (bool|None): Sort directories before files. - dirs_last (bool|None): Sort directories after files. - info_first (bool|None): Prioritize info files. - - Returns: - None - """ - - basename_dict = {pdsf.basename:pdsf for pdsf in self.rows} - - if self.parent_pdsf: - sorted = self.parent_pdsf.sort_basenames(list(basename_dict.keys()), - labels_after=labels_after, - dirs_first=dirs_first, - dirs_last=dirs_last, - info_first=info_first) - - # If there are multiple labels in the group, make sure each label - # is adjacent to any data files of the same name. This can comes up - # with Juno JIRAM, at least. - labels = [basename for basename in sorted - if self.parent_pdsf.basename_is_label(basename)] - if len(labels) > 1: - for label in labels: - l = sorted.index(label) # index of label - pattern = os.path.splitext(label)[0] + '.' - tlist = [t for t in range(len(sorted)) # indices of targets - if sorted[t].startswith(pattern)] - tlist.remove(l) - - # If the label is not adjacent, but could be... - if tlist and (l+1) not in tlist and (l-1) not in tlist: - # Locate the nearest target with a matching basename - diffs = [(abs(t-l),t) for t in tlist] - tbest = min(diffs)[1] - - # Relocate the label to an adjacent location - if l < tbest: - sorted.insert(tbest - 1, sorted.pop(l)) - else: - sorted.insert(tbest + 1, sorted.pop(l)) - - else: - sorted = list(basename_dict.keys()) # for merged directories - sorted.sort() - - self.rows = [basename_dict[key] for key in sorted] - def sort(self, labels_after=None, dirs_first=None, dirs_last=None, info_first=None): """Sort member files by parent-defined rules with tweaks for labels.
379-384: Inconsistent indentation in nested loop.The
forloop body at line 379 uses 2-space indentation while the rest of the codebase uses 4-space indentation.🔧 Proposed fix
if len(labels) > 1: - for label in labels: - l = sorted.index(label) # index of label - pattern = os.path.splitext(label)[0] + '.' - tlist = [t for t in range(len(sorted)) # indices of targets - if sorted[t].startswith(pattern)] - tlist.remove(l) + for label in labels: + l = sorted.index(label) # index of label + pattern = os.path.splitext(label)[0] + '.' + tlist = [t for t in range(len(sorted)) # indices of targets + if sorted[t].startswith(pattern)] + tlist.remove(l)viewmaster/viewmaster.py (2)
2211-2213: Redundant exception object inlogger.exception()call.
logger.exception(e)passes the exception object redundantly sinceexception()automatically captures the current exception info. This is similar to the pattern that was fixed elsewhere in this file.🔧 Proposed fix
except Exception as e: - LOGGER.exception(e) + LOGGER.exception('Cache retrieval failed') pass
1820-1832: Bug:get_query_params_from_dictreturns cleaned original params instead of new_params.Line 1832 returns
clean_query_params(params)but should returnclean_query_params(new_params). The function modifiesnew_paramsbut then ignores it, returning the cleaned originalparamsinstead.🐛 Proposed fix
# Fill in default values for missing keys for key in defaults: if key not in new_params: new_params[key] = defaults[key] - return clean_query_params(params) + return clean_query_params(new_params)
🤖 Fix all issues with AI agents
In `@viewmaster/viewmaster.py`:
- Around line 1383-1388: The timestamp uses naive datetime (start_time =
datetime.datetime.now()) which can cause timezone issues; replace calls to
datetime.datetime.now() with timezone-aware calls (e.g.,
datetime.datetime.now(datetime.timezone.utc)) in this block around start_time
and elapsed calculation and similarly update other occurrences in viewmaster()
and directory_page_html(); ensure get_prev_next_navigation timing and the
logger.warning call still reference the new start_time/elapsed variables and
that imported datetime.timezone is available.
- Around line 2434-2437: The local assignment to PAGE_CACHE shadows the
module-level variable and has no effect; remove the redundant "PAGE_CACHE =
get_page_cache(logger)" line and simply call the cached instance if present
(i.e., if PAGE_CACHE: PAGE_CACHE.flush()), or if the intent truly is to rebind
the module-level PAGE_CACHE, add "global PAGE_CACHE" at the top of the function
and then assign; reference PAGE_CACHE and get_page_cache to locate the code to
change.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
viewmaster/pdsgroup.pyviewmaster/viewmaster.py
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:22.730Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
📚 Learning: 2025-12-18T21:42:57.270Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:21-22
Timestamp: 2025-12-18T21:42:57.270Z
Learning: In viewmaster, use `pdsfile.Pds3File` explicitly in code implementation instead of the generic `pdsfile.PdsFile` base class in pdsgrouptable.py and pdsiterator.py. Docstrings should reference `Pds3File`, with `Pds4File` to be added later when PDS4 is fully supported.
Applied to files:
viewmaster/pdsgroup.pyviewmaster/viewmaster.py
📚 Learning: 2026-01-12T22:07:33.958Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/viewmaster.py:2496-2501
Timestamp: 2026-01-12T22:07:33.958Z
Learning: In viewmaster/viewmaster.py, the if __name__ == '__main__': block that sets debug=True is for development only and should not run in production. Ensure production deployment uses the WSGI server path and that any debug execution is removed or guarded (e.g., do not enable debug mode in the main entry point for production).
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2026-01-12T21:03:22.730Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:22.730Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2025-12-12T23:26:32.767Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: docs/Makefile:11-20
Timestamp: 2025-12-12T23:26:32.767Z
Learning: In the rms-viewmaster repository, the `docs/Makefile` is auto-generated by Sphinx and should not be modified, even if linters flag missing standard phony targets.
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2026-01-12T21:13:24.268Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:460-467
Timestamp: 2026-01-12T21:13:24.268Z
Learning: In viewmaster's pdsgrouptable.py `tables_from_pdsfiles` and similar methods, when a path string cannot be resolved as either Pds3File or Pds4File, the program should be allowed to crash rather than silently skipping the file. This fail-fast approach is preferred for invalid path inputs.
Applied to files:
viewmaster/viewmaster.py
🧬 Code graph analysis (1)
viewmaster/pdsgroup.py (1)
viewmaster/pdsgrouptable.py (3)
iterator(120-126)iterator_for_all(128-134)iterator_for_hidden(136-142)
🪛 ast-grep (0.40.5)
viewmaster/viewmaster.py
[warning] 2546-2546: Detected Flask app with debug=True. Do not deploy to production with this flag enabled as it will leak sensitive information. Instead, consider using Flask configuration variables or setting 'debug' using system environment variables.
Context: app.run(host='0.0.0.0', port=8080, debug=True)
Note: [CWE-489] Active Debug Code. [REFERENCES]
- https://labs.detectify.com/2015/10/02/how-patreon-got-hacked-publicly-exposed-werkzeug-debugger/
(debug-enabled-python)
🪛 Ruff (0.14.11)
viewmaster/pdsgroup.py
1-1: The file is executable but no shebang is present
(EXE002)
33-33: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
33-33: Boolean default positional argument in function definition
(FBT002)
74-74: Missing return type annotation for special method __len__
Add return type annotation: int
(ANN204)
83-83: Missing return type annotation for special method __repr__
Add return type annotation: str
(ANN204)
viewmaster/viewmaster.py
1-1: The file is executable but no shebang is present
(EXE002)
96-96: Boolean positional value in function call
(FBT003)
133-133: Logging statement uses f-string
(G004)
144-144: Logging statement uses f-string
(G004)
263-263: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Avoid specifying long messages outside the exception class
(TRY003)
408-409: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
408-409: Logging statement uses %
(G002)
416-417: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
416-417: Logging statement uses %
(G002)
439-439: Boolean default positional argument in function definition
(FBT002)
1385-1385: datetime.datetime.now() called without a tz argument
(DTZ005)
1387-1387: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1387-1387: Logging statement uses %
(G002)
2228-2228: Abstract raise to an inner function
(TRY301)
2228-2228: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
viewmaster/pdsgroup.py (3)
1-11: LGTM!The module docstring clearly describes the purpose of the
PdsGroupclass and its role in Viewmaster rendering.
33-72: LGTM!The
__init__method correctly handles mutable defaults by usingNoneand initializing inside the function. Theanchorparameter is now properly assigned toself.anchor. Based on learnings, the implementation usesPds3Fileas intended.
558-598: LGTM!The
group_childrenstatic method correctly groups children by anchor and returns properly sortedPdsGroupobjects.viewmaster/viewmaster.py (5)
2138-2153: LGTM! Path traversal vulnerability properly addressed.Using
send_from_directory()correctly prevents path traversal attacks by safely joining paths internally. This addresses the security concern from the previous review.
80-93: LGTM! Explicit imports replace wildcard import.The change from
from .viewmaster_config import *to explicit named imports improves code clarity and eliminates static analysis warnings (F403/F405).
102-167: LGTM! Logger creation pattern is well-structured.The separation of
create_logger()as a pure factory andget_or_create_logger()as a singleton accessor is a clean design that prevents duplicate handlers and enables testing flexibility.
386-431: LGTM! Memcache fallback uses local variable correctly.The
UnboundLocalErrorissue is fixed by using a localmemcache_portvariable instead of reassigning the globalVIEWMASTER_MEMCACHE_PORT.
2538-2547: Development-only debug mode acknowledged.Per repository learnings, the
debug=Truein the__main__block is intentionally kept for development. Production uses a WSGI server which bypasses this code path.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| start_time = datetime.datetime.now() | ||
| (page['prev'], page['next']) = get_prev_next_navigation(query_pdsfile) | ||
| (page['prev'], page['next']) = get_prev_next_navigation(query_pdsfile, logger) | ||
| elapsed = (datetime.datetime.now() - start_time).total_seconds() | ||
| if elapsed > 10: | ||
| LOGGER.warn('Neighbor navigation took %.1f sec' % elapsed, | ||
| query_pdsfile.abspath) | ||
| logger.warning('Neighbor navigation took %.1f sec' % elapsed, | ||
| query_pdsfile.abspath) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding timezone to datetime.now() call.
datetime.datetime.now() is called without a timezone argument, which could cause issues in timezone-aware environments. Consider using datetime.datetime.now(datetime.timezone.utc) for consistency.
♻️ Suggested improvement
+import datetime
+
# Get neighbor navigation and warn about timing if it is very slow
- start_time = datetime.datetime.now()
+ start_time = datetime.datetime.now(datetime.timezone.utc)
(page['prev'], page['next']) = get_prev_next_navigation(query_pdsfile, logger)
- elapsed = (datetime.datetime.now() - start_time).total_seconds()
+ elapsed = (datetime.datetime.now(datetime.timezone.utc) - start_time).total_seconds()This pattern should also be applied to the other datetime.datetime.now() calls in viewmaster() and directory_page_html().
🧰 Tools
🪛 Ruff (0.14.11)
1383-1383: datetime.datetime.now() called without a tz argument
(DTZ005)
1385-1385: datetime.datetime.now() called without a tz argument
(DTZ005)
1387-1387: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1387-1387: Logging statement uses %
(G002)
🤖 Prompt for AI Agents
In `@viewmaster/viewmaster.py` around lines 1383 - 1388, The timestamp uses naive
datetime (start_time = datetime.datetime.now()) which can cause timezone issues;
replace calls to datetime.datetime.now() with timezone-aware calls (e.g.,
datetime.datetime.now(datetime.timezone.utc)) in this block around start_time
and elapsed calculation and similarly update other occurrences in viewmaster()
and directory_page_html(); ensure get_prev_next_navigation timing and the
logger.warning call still reference the new start_time/elapsed variables and
that imported datetime.timezone is available.
There was a problem hiding this comment.
@juzen2003, understood! I'll leave the datetime.datetime.now() calls as-is without timezone arguments.
🧠 Learnings used
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:22.730Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
Updated the fill_page_cache function to declare PAGE_CACHE as a global variable, ensuring it is properly referenced within the function. This change enhances the clarity of cache management and maintains consistency in variable usage across the module.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
viewmaster/viewmaster.py (2)
666-684: Critical bug:row_pdsfileis referenced but never defined.Line 673 references
row_pdsfile, but this variable is never defined inget_prev_next_navigation(). This will cause aNameErrorat runtime when processing a nonexistent index row.Based on the context, it appears
row_pdsfileshould likely be derived fromquery_pdsfileor obtained through some other means before being used.Likely fix (needs verification)
# Deal with a nonexistent index row--it still has neighbors! if query_pdsfile.is_index_row and not query_pdsfile.exists: # Note that, in this case, each list begins with query_pdsfile, but # that row_pdsfile is missing from both lists. We need to put it back # in the proper place - row_pdsfile = row_pdsfile.copy() + # If the intent is to work with query_pdsfile: + row_pdsfile = query_pdsfile.copy() row_pdsfile.nav_name = row_pdsfile.basename row_pdsfile.division = False row_pdsfile.terminated = FalsePlease verify the intended behavior and the correct source for
row_pdsfile.
2211-2213: Minor: Redundant exception object inlogger.exception()call.Line 2212 passes the exception object
etologger.exception(), butexception()automatically captures exception info. This is redundant but not harmful.Optional cleanup
except Exception as e: - LOGGER.exception(e) + LOGGER.exception('Cache retrieval failed') pass
🤖 Fix all issues with AI agents
In `@viewmaster/viewmaster.py`:
- Line 33: Replace the deprecated cgi usage: remove the "import cgi" and add
"import html" at the top of viewmaster.py, then replace all calls to cgi.escape
with html.escape (preserve the existing second arg/quote behavior, e.g.
cgi.escape(s, quote=True) -> html.escape(s, quote=True)); specifically update
the cgi.escape occurrences noted around lines 1106, 2039, and 2052 to use
html.escape.
- Around line 2376-2388: The function fill_page_cache(logger) declares a logger
parameter but never uses it, causing an API inconsistency; either remove the
unused logger parameter from fill_page_cache and update all call sites that pass
a logger to call fill_page_cache() with no args, or keep the parameter and
replace internal uses of the global LOGGER/viewmaster() logging with direct
calls to the passed-in logger (e.g., logger.info/error) inside fill_page_cache;
also ensure any callers that expect the old signature continue to pass or stop
passing the logger to match your chosen change.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
viewmaster/viewmaster.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-01-12T22:07:33.958Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/viewmaster.py:2496-2501
Timestamp: 2026-01-12T22:07:33.958Z
Learning: In viewmaster/viewmaster.py, the if __name__ == '__main__': block that sets debug=True is for development only and should not run in production. Ensure production deployment uses the WSGI server path and that any debug execution is removed or guarded (e.g., do not enable debug mode in the main entry point for production).
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2026-01-12T21:03:22.730Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:22.730Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2025-12-12T23:26:32.767Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: docs/Makefile:11-20
Timestamp: 2025-12-12T23:26:32.767Z
Learning: In the rms-viewmaster repository, the `docs/Makefile` is auto-generated by Sphinx and should not be modified, even if linters flag missing standard phony targets.
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2025-12-18T21:42:57.270Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:21-22
Timestamp: 2025-12-18T21:42:57.270Z
Learning: In viewmaster, use `pdsfile.Pds3File` explicitly in code implementation instead of the generic `pdsfile.PdsFile` base class in pdsgrouptable.py and pdsiterator.py. Docstrings should reference `Pds3File`, with `Pds4File` to be added later when PDS4 is fully supported.
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2026-01-12T21:13:24.268Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:460-467
Timestamp: 2026-01-12T21:13:24.268Z
Learning: In viewmaster's pdsgrouptable.py `tables_from_pdsfiles` and similar methods, when a path string cannot be resolved as either Pds3File or Pds4File, the program should be allowed to crash rather than silently skipping the file. This fail-fast approach is preferred for invalid path inputs.
Applied to files:
viewmaster/viewmaster.py
🪛 ast-grep (0.40.5)
viewmaster/viewmaster.py
[warning] 2546-2546: Detected Flask app with debug=True. Do not deploy to production with this flag enabled as it will leak sensitive information. Instead, consider using Flask configuration variables or setting 'debug' using system environment variables.
Context: app.run(host='0.0.0.0', port=8080, debug=True)
Note: [CWE-489] Active Debug Code. [REFERENCES]
- https://labs.detectify.com/2015/10/02/how-patreon-got-hacked-publicly-exposed-werkzeug-debugger/
(debug-enabled-python)
🪛 Ruff (0.14.11)
viewmaster/viewmaster.py
1-1: The file is executable but no shebang is present
(EXE002)
96-96: Boolean positional value in function call
(FBT003)
133-133: Logging statement uses f-string
(G004)
144-144: Logging statement uses f-string
(G004)
263-263: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Avoid specifying long messages outside the exception class
(TRY003)
408-409: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
408-409: Logging statement uses %
(G002)
416-417: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
416-417: Logging statement uses %
(G002)
439-439: Boolean default positional argument in function definition
(FBT002)
1385-1385: datetime.datetime.now() called without a tz argument
(DTZ005)
1387-1387: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1387-1387: Logging statement uses %
(G002)
2228-2228: Abstract raise to an inner function
(TRY301)
2228-2228: Avoid specifying long messages outside the exception class
(TRY003)
2376-2376: Unused function argument: logger
(ARG001)
🔇 Additional comments (5)
viewmaster/viewmaster.py (5)
102-167: LGTM!The logger creation pattern is well-structured with proper separation between the factory (
create_logger()) and singleton accessor (get_or_create_logger()). The duplicate handler protection viahas_handlerscheck and graceful error handling for file operations are appropriate.
249-263: LGTM!The environment variable-based holdings path retrieval is cleaner than the previous config file approach. Clear error messaging when
PDS3_HOLDINGS_DIRis not set.
386-431: LGTM!The cache initialization correctly uses a local
memcache_portvariable to avoid the previousUnboundLocalErrorissue. The fallback fromMemcachedCachetoDictionaryCacheon connection failure provides good resilience.
2538-2547: LGTM!The main entry point correctly initializes the logger, loads icons, sets up holdings paths and page cache, then initializes caches before starting the app. The
debug=Trueis acceptable for development-only execution. Based on learnings, production deployment uses a WSGI server rather than this entry point.
2138-2153: LGTM!The path traversal vulnerability has been correctly addressed by using
send_from_directory()instead of manually constructing file paths. Flask'ssend_from_directory()internally validates that the resolved path stays within the specified root directory.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Updated the code to replace cgi.escape with html.escape for escaping parameters in the directory_page_html and url_params functions. This change improves security and aligns with modern Python practices for handling HTML content.
Updated the fill_page_cache function to eliminate the logger parameter, simplifying its signature. This change enhances the function's usability and aligns with the current logging practices in the module.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
viewmaster/viewmaster.py (4)
454-456: Remove debugLine 455 contains a
🔧 Proposed fix
LOGGER.replace_root(HOLDINGS_PATHS) - print(VIEWMASTER_PREFIX_+ICON_URL_) + LOGGER.debug('Icon URL prefix: %s', VIEWMASTER_PREFIX_ + ICON_URL_) Pds3File.preload(HOLDINGS_PATHS, port=PDSFILE_MEMCACHE_PORT, clear=reset, icon_url=ICON_URL_)
2211-2213: Redundant exception object inLOGGER.exception()call.
LOGGER.exception(e)on line 2212 passes the exception object unnecessarily. Theexception()method automatically captures exception information from the current context.♻️ Proposed fix
except Exception as e: - LOGGER.exception(e) + LOGGER.exception('Cache retrieval failed') pass
1821-1832: RuntimeError: Dictionary modification during iteration.Deleting keys from
new_paramswhile iterating over it will raiseRuntimeError: dictionary changed size during iteration. Use a list comprehension or iterate over a copy.🐛 Proposed fix
# Remove unrecognized keys new_params = params.copy() - for key in new_params: - if key not in defaults: - del new_params[key] + new_params = {k: v for k, v in new_params.items() if k in defaults} # Fill in default values for missing keys for key in defaults: if key not in new_params: new_params[key] = defaults[key] - return clean_query_params(params) + return clean_query_params(new_params)Note: Line 1832 also has a bug - it should pass
new_paramstoclean_query_params, not the originalparams.
2064-2086: Fix WTForms validator signature to accept (form, field).The
pattern_validatorfunction signature is incompatible with WTForms. Validators in WTForms must accept two arguments:(form, field). The current signaturepattern_validator(field)will cause aTypeErrorat runtime when form validation is triggered, as WTForms will pass both the form and field but the validator only accepts one parameter.Update the signature to
def pattern_validator(form, field):at line 2064.
🤖 Fix all issues with AI agents
In `@viewmaster/viewmaster.py`:
- Around line 380-381: Replace the fragile assertion with explicit validation:
instead of using "assert len(paths) == 1", check "if len(paths) != 1:" and raise
a clear exception (e.g., ValueError("expected exactly one path in paths, got
{len(paths)}")) or log an error and exit; then assign HOLDINGS_PATHS = paths
only after the check succeeds. Ensure you update the code around the symbols
"paths" and "HOLDINGS_PATHS" and remove the assert so the validation runs even
with Python -O.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
viewmaster/viewmaster.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-01-12T22:07:33.958Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/viewmaster.py:2496-2501
Timestamp: 2026-01-12T22:07:33.958Z
Learning: In viewmaster/viewmaster.py, the if __name__ == '__main__': block that sets debug=True is for development only and should not run in production. Ensure production deployment uses the WSGI server path and that any debug execution is removed or guarded (e.g., do not enable debug mode in the main entry point for production).
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2026-01-12T21:03:22.730Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: README.md:16-18
Timestamp: 2026-01-12T21:03:22.730Z
Learning: In the rms-viewmaster repository, the README.md format should match the README format used in other SETI repositories (such as rms-starcat) for consistency, even if it results in MD031 linting violations (missing blank lines around fenced code blocks).
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2025-12-12T23:26:32.767Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: docs/Makefile:11-20
Timestamp: 2025-12-12T23:26:32.767Z
Learning: In the rms-viewmaster repository, the `docs/Makefile` is auto-generated by Sphinx and should not be modified, even if linters flag missing standard phony targets.
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2025-12-18T21:42:57.270Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:21-22
Timestamp: 2025-12-18T21:42:57.270Z
Learning: In viewmaster, use `pdsfile.Pds3File` explicitly in code implementation instead of the generic `pdsfile.PdsFile` base class in pdsgrouptable.py and pdsiterator.py. Docstrings should reference `Pds3File`, with `Pds4File` to be added later when PDS4 is fully supported.
Applied to files:
viewmaster/viewmaster.py
📚 Learning: 2026-01-12T21:13:24.268Z
Learnt from: juzen2003
Repo: SETI/rms-viewmaster PR: 14
File: viewmaster/pdsgrouptable.py:460-467
Timestamp: 2026-01-12T21:13:24.268Z
Learning: In viewmaster's pdsgrouptable.py `tables_from_pdsfiles` and similar methods, when a path string cannot be resolved as either Pds3File or Pds4File, the program should be allowed to crash rather than silently skipping the file. This fail-fast approach is preferred for invalid path inputs.
Applied to files:
viewmaster/viewmaster.py
🪛 ast-grep (0.40.5)
viewmaster/viewmaster.py
[warning] 2543-2543: Detected Flask app with debug=True. Do not deploy to production with this flag enabled as it will leak sensitive information. Instead, consider using Flask configuration variables or setting 'debug' using system environment variables.
Context: app.run(host='0.0.0.0', port=8080, debug=True)
Note: [CWE-489] Active Debug Code. [REFERENCES]
- https://labs.detectify.com/2015/10/02/how-patreon-got-hacked-publicly-exposed-werkzeug-debugger/
(debug-enabled-python)
🪛 Ruff (0.14.11)
viewmaster/viewmaster.py
1-1: The file is executable but no shebang is present
(EXE002)
96-96: Boolean positional value in function call
(FBT003)
133-133: Logging statement uses f-string
(G004)
144-144: Logging statement uses f-string
(G004)
263-263: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Avoid specifying long messages outside the exception class
(TRY003)
408-409: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
408-409: Logging statement uses %
(G002)
416-417: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
416-417: Logging statement uses %
(G002)
439-439: Boolean default positional argument in function definition
(FBT002)
1385-1385: datetime.datetime.now() called without a tz argument
(DTZ005)
1387-1387: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1387-1387: Logging statement uses %
(G002)
2228-2228: Abstract raise to an inner function
(TRY301)
2228-2228: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
viewmaster/viewmaster.py (8)
20-56: LGTM! Well-organized imports.The import reorganization is clean:
- Flask imports properly grouped with
send_from_directoryfor secure file servinghtmlmodule correctly replaces the deprecatedcgimodule- Relative imports (
.pdsgroup,.pdsgrouptable,.pdsiterator) are appropriate for package structure
80-100: LGTM! Explicit imports and lazy initialization pattern.The explicit imports from
viewmaster_configeliminate static analysis warnings and make dependencies clear. The global variables (LOGGER,HOLDINGS_PATHS,PAGE_CACHE) are properly initialized toNonefor lazy initialization.
102-167: LGTM! Clean logger implementation with proper safeguards.The logger implementation properly addresses previous concerns:
has_handlerscheck (line 122) prevents duplicate handlers on logger reuse- OSError handling (lines 132-133, 143-144) prevents crashes on file-open failures
get_or_create_logger()provides a clean singleton pattern
249-348: LGTM! Holdings path validation with consistent error handling.The implementation correctly:
- Uses
OSErrorconsistently (replacing deprecatedIOError)- Accepts
loggeras a parameter for explicit dependency injection- Validates holdings paths with appropriate logging for various failure modes
573-588: LGTM! Consistent logger parameter threading.The logger is properly passed as a parameter through the rendering function chain (
get_prev_next_navigation,get_directory_page,directory_page_html,get_product_page_info,product_page_html), enabling explicit dependency injection and avoiding reliance on global state within these functions.
2148-2153: LGTM! Path traversal vulnerability fixed.Using
send_from_directory()properly prevents path traversal attacks by safely joining the root directory with the query path.
2376-2434: LGTM! Cache warming implementation.The
fill_page_cache()function properly uses the globalPAGE_CACHEand walks the holdings tree to warm commonly accessed pages.
2535-2544: LGTM! Development entry point with proper initialization.The
__main__block correctly sequences initialization: logger → icons → holdings path → page cache → caches → app start. Based on learnings,debug=Trueis intentional for development use only, as production deployment uses a WSGI server that bypasses this code path.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
viewmaster/viewmaster.py
Outdated
| assert len(paths) == 1 | ||
| HOLDINGS_PATHS = paths |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider replacing assert with explicit validation.
The assert statement can be disabled when Python runs with the -O (optimize) flag, which would silently skip this check in production. For critical validation, use an explicit check:
♻️ Proposed fix
- assert len(paths) == 1
- HOLDINGS_PATHS = paths
+ if len(paths) != 1:
+ raise RuntimeError(f'Expected exactly one holdings path, got {len(paths)}')
+ HOLDINGS_PATHS = paths🤖 Prompt for AI Agents
In `@viewmaster/viewmaster.py` around lines 380 - 381, Replace the fragile
assertion with explicit validation: instead of using "assert len(paths) == 1",
check "if len(paths) != 1:" and raise a clear exception (e.g.,
ValueError("expected exactly one path in paths, got {len(paths)}")) or log an
error and exit; then assign HOLDINGS_PATHS = paths only after the check
succeeds. Ensure you update the code around the symbols "paths" and
"HOLDINGS_PATHS" and remove the assert so the validation runs even with Python
-O.
Modified the get_holdings_path function to raise a RuntimeError if the number of holdings paths is not exactly one, improving error handling and clarity in path validation. This change enhances the robustness of the function by ensuring it meets expected conditions before proceeding.
This pull is to address #9 and #12, and it's currently in progress.
Changes:
README.mdrequirments.txtto include Sphinx related packagesrms-viewmaster/docsrms-viewmaster/docs/_build.readthedocs.yamlfor Read the docs (build successed, verified on forked repo)rms-viewmaster/viewmaster/docviewmaster_without_pause.pyand its related documentation files..rstfile to the same as the title length. Note: Sphinx only complains when the header length is shorter than the title length.README.mdto match the format of the one fromrms-starcatrequirements.txtto have all packages in alphabetical orderlink.pyandviewmaster.pyto move the step of creating logger from global level to a function. Docstrings are updated correspondingly.Pending items:
pip install rms-viewmaster.githubdirectory with the appropriate actions for uploading to PyPI and TestPyPI, running lint, and hopefully some day running tests.Summary by CodeRabbit
New Features
Documentation
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.