feat: walkthrough documents with reading progress and GameFAQs import#3816
feat: walkthrough documents with reading progress and GameFAQs import#3816gantoine wants to merge 1 commit into
Conversation
Unify manuals and walkthroughs on one document model instead of a separate walkthrough subsystem. Walkthroughs are RomFiles under a new WALKTHROUGH category with an optional RomFileDocMeta provenance sidecar, and reading progress is a per-user RomFileUser row keyed on rom_file_id so the same mechanism covers manuals and walkthroughs. Backend: - models: RomFileCategory.WALKTHROUGH, DocSource, RomFileDocMeta, RomFileUser; migration 0098 (enum value + two tables, cross-DB) - serve .txt/.html documents inline (HTML under a sandboxing CSP); add .txt to allowed manual files - endpoints: walkthrough upload / GameFAQs fetch / delete, and per-user reading-progress GET/PUT (ROMS_READ, like notes) - GameFAQs import extracts text only (stored as .txt); no remote HTML is persisted or served. Host-allowlisted on top of the SSRF-safe client - tests for CRUD, progress, and the GameFAQs parser/URL validation Frontend (v2): - TextViewer (.txt/.html), useReadingProgress composable, WalkthroughSubtab (upload + add-from-GameFAQs-URL + delete + progress), wired into MediaTab - ManualSubtab renders .txt manuals; walkthrough category surfaced in FilesTab; regenerated types; i18n keys across all locales AI assistance: implemented with Claude Code (Opus 4.8). The data-model design, backend, migration, tests, and v2 UI were AI-authored under human direction. Alternative approach to #2892. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR adds walkthrough documents and shared reading progress for manuals and walkthroughs. The main changes are:
Confidence Score: 4/5Hidden-ROM mutations, failed replacement uploads, and lost progress saves need fixes before merging.
backend/endpoints/roms/walkthrough.py, backend/handler/database/roms_handler.py, frontend/src/v2/composables/useReadingProgress/index.ts
|
| Filename | Overview |
|---|---|
| backend/endpoints/roms/walkthrough.py | Adds walkthrough mutations and GameFAQs import, with visibility and filesystem consistency gaps. |
| backend/endpoints/roms/files.py | Adds guarded document serving and per-user reading-progress endpoints. |
| backend/handler/database/roms_handler.py | Adds document metadata and progress persistence, including a race-prone read-then-insert upsert. |
| backend/handler/walkthrough/gamefaqs.py | Adds allowlisted GameFAQs fetching and plain-text guide extraction. |
| backend/alembic/versions/0098_walkthrough_docs.py | Adds the walkthrough enum value and document metadata and progress tables. |
| backend/models/rom.py | Adds document provenance, walkthrough category, and per-file user progress models. |
| frontend/src/v2/composables/useReadingProgress/index.ts | Adds debounced progress persistence but drops pending state during document switches. |
| frontend/src/v2/components/GameDetails/TextViewer.vue | Adds plain-text rendering, sandboxed HTML viewing, and progress integration. |
| frontend/src/v2/components/GameDetails/WalkthroughSubtab.vue | Adds walkthrough selection, upload, import, deletion, and viewing controls. |
Reviews (1): Last reviewed commit: "feat: walkthrough documents with reading..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| rom = db_rom_handler.get_rom(id) | ||
| if not rom: | ||
| raise RomNotFoundInDatabaseException(id) | ||
|
|
There was a problem hiding this comment.
These routes resolve the ROM but never call assert_rom_visible. A user with ROMS_WRITE who knows a hidden ROM ID can upload or import a walkthrough, and the delete route has the same gap, allowing files on a hidden ROM to be modified or removed.
| await fs_rom_handler.make_directory(walkthrough_dir_rel) | ||
|
|
There was a problem hiding this comment.
FileTarget writes directly to the final path, including when a walkthrough with that name already exists. If the client disconnects or multipart parsing fails, the error handler unlinks that path, so an interrupted replacement permanently removes the previous valid document while its database row remains.
| created = db_rom_handler.add_rom_file( | ||
| RomFile( | ||
| rom_id=rom.id, | ||
| file_name=file_name, | ||
| file_path=walkthrough_dir_rel, | ||
| file_size_bytes=stat.st_size, | ||
| last_modified=stat.st_mtime, | ||
| category=RomFileCategory.WALKTHROUGH, | ||
| ) | ||
| ) | ||
| db_rom_handler.upsert_doc_meta( | ||
| rom_file_id=created.id, | ||
| rom_id=rom.id, | ||
| values={ | ||
| "source": DocSource.GAMEFAQS, | ||
| "source_url": url, | ||
| "author": guide["author"], | ||
| "title": guide["title"], | ||
| }, | ||
| ) |
There was a problem hiding this comment.
The guide is written before its RomFile and metadata rows are created. If either database operation fails, the request returns an error but leaves an unindexed file or a file row without its intended provenance metadata; the upload route has the same filesystem-first failure path.
| @begin_session | ||
| def upsert_rom_file_user( | ||
| self, | ||
| rom_file_id: int, | ||
| user_id: int, | ||
| values: dict, | ||
| session: Session = None, # type: ignore | ||
| ) -> RomFileUser: | ||
| """Create or update a user's reading progress for a document file.""" | ||
| existing = session.scalar( | ||
| select(RomFileUser) | ||
| .filter_by(rom_file_id=rom_file_id, user_id=user_id) | ||
| .limit(1) | ||
| ) | ||
| if existing: | ||
| for key, val in values.items(): | ||
| setattr(existing, key, val) | ||
| session.flush() | ||
| return existing | ||
|
|
||
| state = RomFileUser(rom_file_id=rom_file_id, user_id=user_id, **values) | ||
| session.add(state) | ||
| session.flush() | ||
| return state |
There was a problem hiding this comment.
This upsert performs a read followed by an insert without locking or handling the unique constraint. Two saves for the same user and file, such as concurrent browser tabs, can both observe no row and then race to insert, causing one progress request to fail instead of updating the existing state.
| watch(fileId, () => { | ||
| if (saveTimer) { | ||
| clearTimeout(saveTimer); | ||
| saveTimer = null; | ||
| } | ||
| progress.value = 0; | ||
| void restore(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
When fileId changes within the debounce window, this watcher clears the old document's timer without saving its pending position. The viewer then restores the new document, so the user's latest position in the previous document is permanently lost; the pending value must retain its original ROM and file identity and be flushed before switching.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Description
Explain the changes or enhancements you are proposing with this pull request.
Adds walkthrough documents to RomM and unifies them with manuals on a single document model, rather than building walkthroughs as a separate subsystem (an alternative approach to #2892).
The key idea: manuals already have a many-per-ROM substrate (
rom_fileswithcategory=manual). Walkthroughs becomerom_filesunder a newwalkthroughcategory, with an optionalRomFileDocMetaprovenance sidecar, and reading progress is a per-userRomFileUserrow keyed onrom_file_id— so one mechanism covers manuals and walkthroughs. The legacy scraped-manual columns onromsare untouched.Backend
RomFileCategory.WALKTHROUGH,DocSourceenum,RomFileDocMetasidecar (mirrorsTrackMeta),RomFileUserprogress table (mirrorsRomUser).0098_walkthrough_docs— enum value + two tables, cross-DB (Postgres/MySQL/MariaDB); upgrade→downgrade→re-upgrade verified..txt/.htmldocuments inline; HTML is served under a sandboxingContent-Security-Policy..txtadded to allowed manual files.GET/PUT(usesROMS_READ, since progress is personal state like notes)..txt) — no remote HTML is ever persisted or served, so sanitization is by construction. Requests are host-allowlisted on top of the existing SSRF-protected client.Frontend (v2)
TextViewer(.txt/.html),useReadingProgresscomposable,WalkthroughSubtab(upload + add-from-GameFAQs-URL + delete + reading-progress bar), wired intoMediaTab.ManualSubtabnow renders.txtmanuals; thewalkthroughcategory is surfaced inFilesTab. Types regenerated; i18n keys added across all locales.AI assistance disclosure
Implemented with Claude Code (Opus 4.8). The data-model design, backend, migration, tests, and v2 UI were AI-authored under human direction. Per RomM's contribution policy.
Checklist
Please check all that apply.
Screenshots (if applicable)
Pending browser QA.