feat(sage): cablea el sabio como NPC + leyenda de glifos (#11) - #50
feat(sage): cablea el sabio como NPC + leyenda de glifos (#11)#50blippip69 wants to merge 1 commit into
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| case 'sage': | ||
| this.sageSession = new SageSession( | ||
| null, | ||
| () => { | ||
| try { return fs.readFileSync(SCRIPT_PATH, 'utf8') } catch { return '' } | ||
| }, | ||
| (next) => fs.writeFileSync(SCRIPT_PATH, next), | ||
| (lines) => { | ||
| for (const line of lines || []) this.say(line) | ||
| if (this.activeSlot) this.loadScript(true) | ||
| this.sageSession = null | ||
| } | ||
| ).start() | ||
| break |
There was a problem hiding this comment.
🚨 Bug: Sage NPC is unreachable — interactNpc ignores kind 'sage'
The sabio is placed as a CITY_NPC with action: { kind: 'sage' } (map.js), but NPCs are activated through interactNpc, which only handles shop/church/tavern and silently returns for anything else. The case 'sage' block that actually starts the SageSession lives in enter()'s tile switch, which is only reached via walker.action() (tile enter descriptors) — no tile carries kind: 'sage'. So talking to the sabio just prints his line and never opens the conversation, leaving the feature (the PR's main goal) unreachable. Add a sage branch to interactNpc (e.g. call this.startSage()), which already handles the say-line display and reload wiring.
Route NPC 'sage' action to startSage() inside interactNpc.:
if (action.kind === 'tavern') {
this.restAtTavern()
return true
}
if (action.kind === 'sage') {
this.startSage()
return true
}
return true
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| if (this.legendOpen) { | ||
| const { LEGEND } = require('./map.js') | ||
| const rows = Object.entries(LEGEND || {}).map(([glyph, meaning]) => `${glyph} ${meaning}`) |
There was a problem hiding this comment.
⚠️ Bug: LEGEND is an array but rendered via Object.entries
In view() the legend overlay does Object.entries(LEGEND || {}).map(([glyph, meaning]) => ${glyph} ${meaning}), but LEGEND in map.js is an array of { glyph, text } objects. Object.entries on an array yields index keys and object values, so every row renders as 0 [object Object], 1 [object Object], … instead of the intended glyph/meaning. Iterate the array and read the fields explicitly.
Map the array of {glyph,text} entries directly.:
const rows = (LEGEND || []).map((e) => `${e.glyph} ${e.text}`)
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| // Map glyph legend (issue #11): the exported LEGEND finally has a reader. | ||
| if (key.matches(msg, 'l') && !this.field) { | ||
| this.legendOpen = true | ||
| return null | ||
| } | ||
| if (this.legendOpen) { | ||
| if (key.matches(msg, 'escape', 'l', 'enter')) { | ||
| this.legendOpen = false | ||
| this.say('cerraste la leyenda de glifos') | ||
| } else { | ||
| this.say('esc o l para cerrar la leyenda') | ||
| } | ||
| return null | ||
| } | ||
|
|
There was a problem hiding this comment.
⚠️ Bug: 'l' key hijacks sage input and opens the legend
In onKey, the legend toggle key.matches(msg, 'l') && !this.field is evaluated before the sageSession input branch. While the sage conversation is open in town, pressing the letter 'l' opens the legend overlay instead of appending to the sage buffer, so any sentence containing 'l' (extremely common in Spanish: 'la', 'el', 'ballesta') cannot be typed. Gate the legend toggle so it does not fire while a sage session is active (or move the sage/legend checks so an active session takes precedence).
Don't treat 'l' as the legend toggle while the sage is listening.:
if (key.matches(msg, 'l') && !this.field && !(this.sageSession && this.sageSession.active)) {
this.legendOpen = true
return null
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| sageKey(msg) { | ||
| const ch = typeof msg === 'string' ? msg : (msg && msg.key) || '' | ||
| if (!ch) return null | ||
| if (ch === 'enter' || ch === 'return') { | ||
| const sentence = this.sageBuffer.trim() | ||
| this.sageBuffer = '' | ||
| if (sentence) this.sageSession.ask(sentence) | ||
| else this.sageSession.close('el sabio espera una frase') | ||
| return null | ||
| } | ||
| if (ch === 'backspace' || ch === 'delete') { | ||
| this.sageBuffer = this.sageBuffer.slice(0, -1) | ||
| return render.sagePrompt(this.sageBuffer) | ||
| } | ||
| if (ch.length === 1) { |
There was a problem hiding this comment.
⚠️ Bug: sageKey reads msg.key; printable chars use msg.sequence
sageKey extracts the character with (msg && msg.key) || '', but key events in this codebase carry the typed character in msg.sequence (see the test harness typeText, which sends { type: 'key', sequence, ... }, and the fact that all other key routing uses key.matches). Reading msg.key likely yields undefined for printable input, so the sage buffer never fills and enter/backspace comparisons also fail. There is no integration test covering this path, so it would go unnoticed. Use msg.sequence for the character and key.matches(msg, 'enter'/'backspace') for control keys.
Was this helpful? React with 👍 / 👎
| startSage() { | ||
| this.sageSession = new SageSession( | ||
| null, | ||
| () => { | ||
| try { return fs.readFileSync(SCRIPT_PATH, 'utf8') } catch { return '' } | ||
| }, | ||
| (next) => fs.writeFileSync(SCRIPT_PATH, next), | ||
| (lines) => { | ||
| for (const line of lines || []) this.say(line) | ||
| if (this.activeSlot) this.loadScript(true) | ||
| this.sageSession = null | ||
| } | ||
| ).start() | ||
| for (const line of this.sageSession.say || []) this.say(line) | ||
| return this.sageSession |
There was a problem hiding this comment.
💡 Quality: NPC 'sage' path doesn't show the sage's opening lines
The case 'sage' block in enter() builds and .start()s the SageSession but, unlike startSage(), never iterates this.sageSession.say to push the intro lines to the log. If the sage is entered through this path the player gets no on-screen prompt that the sage is listening. Prefer calling startSage() from both entry points to avoid the duplicated (and divergent) session-construction logic.
Was this helpful? React with 👍 / 👎
| * by the normal script reload path. | ||
| */ | ||
|
|
||
| const fs = require('bare-fs') |
There was a problem hiding this comment.
💡 Quality: Unused fs import in sage-npc.js
sage-npc.js requires bare-fs at the top, but SageSession receives its read/write functions via constructor injection and never touches fs directly. The import is dead and can be removed to keep the module's IO boundary honest.
Was this helpful? React with 👍 / 👎
CI failed: Linting failed because Prettier found code style issues in 3 files.OverviewPrettier code formatting checks failed during the lint job, identifying style issues in 3 files. Total logs analyzed: 1. FailuresPrettier Formatting Check Failed (confidence: high)
Summary
Code Review 🚫 Blocked 0 resolved / 6 findingsWires up the sage NPC and glyph legend overlay, but the NPC interaction is currently unreachable, the legend rendering crashes due to type mismatch, and several keyboard handling bugs require attention. 🚨 Bug: Sage NPC is unreachable — interactNpc ignores kind 'sage'📄 lib/game.js:533-547 📄 lib/game.js:1304-1317 📄 lib/map.js:374-385 The sabio is placed as a CITY_NPC with Route NPC 'sage' action to startSage() inside interactNpc.
|
| Auto-apply | Compact |
|
|
Important
Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.
Was this helpful? React with 👍 / 👎 | Gitar
|
Te dejo un mensaje que aplica a todos tus PRs abiertos, para no repetirlo seis veces. Lo primero: tu trabajo entraTu PR #45, el arbitro Soroban de commit-reveal para los duelos, ya esta en Estas a un comando de cinco mergesHabilite la ejecucion del CI en tus PRs, que estaba trabada esperando aprobacion de En los cinco primeros falla unicamente Los siete builds de plataforma pasan. Es formato del archivo de test que agregaste, El #38 es distinto, ahi hay un problema realEse no falla por formato: el archivo de tests no parsea, asi que la suite entera El test nuevo quedo insertado antes del Dicho sea de paso: el arreglo del #38 en si es el mejor de los dos que llegaron Sobre la walletVi que la pegas en los comentarios. No hace falta, y lo aclaro porque el problema era El pago lo maneja enteramente la plataforma de la campana. Una direccion en un Una sugerencia, y es solo esoTenes 36 PRs abiertos en la organizacion. Ninguno es malo, pero revisarlos lleva Cuando empujes los cambios avisá por aca y los vuelvo a correr. |
2 participants
feat(sage): cablea el sabio como NPC + leyenda de glifos (#11)
Que se conecta
lib/sage.jsdeja de estar huerfano (1494 lineas testeadas que eljugador no podia alcanzar):
action { kind: 'sage' }),lib/map.js.lib/sage-npc.js(nuevo):SageSession— traduce una frase viaSage.ask(), agrega la regla resultante ascript.txty recarga por elcamino normal (
loadScript(true)), asi el script vive donde siempre.?sigue funcionando como camino manual; el sabio es ahora laentrada principal que la issue pedia.
LEGENDconsigue lector: teclalen la ciudad abre un overlay con losglifos del mapa (esc/l cierra). Era exportada y no consumida por nadie.
JSDoc huerfano en
lib/game.jsremovido junto al estado nuevo documentado.Tests
frases sin sentido no escriben nada y la sesion cierra limpia,
Partido del main de hoy como pedia la issue. El resto del inventario de arte
muerto (
makeCityRows,market(), sprites legacy) queda para un PR siguientepara mantener este revisable.