Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Bulletin Manager feature: DB migration and tables for bulletins/read-tracking, a PDO-backed BulletinManager with API and admin CRUD/reorder, web and telnet/SSH viewers with unread tracking/display-mode config, a browser Markdown-editor wrapper, service-worker precache update, i18n, terminal charset handling, and upgrade docs. ChangesBulletin Manager System
Migration & Tooling / Versioning & Docs
Sequence Diagram(s)sequenceDiagram
participant Browser as Web Browser
participant Server as App Server
participant API as Bulletin API
participant DB as Database
Browser->>Server: GET /
Server->>API: Request unread bulletin count
API->>DB: SELECT unread active bulletins/count
DB-->>API: count
API-->>Server: unread_count
Server->>Browser: 200 or 302 /bulletins (based on display_mode + session)
alt Redirected
Browser->>Server: GET /bulletins
Server->>API: GET /api/bulletins (unread or all)
API->>DB: SELECT bulletins (+ read state)
DB-->>API: bulletins[]
API-->>Server: bulletins
Server-->>Browser: render bulletins.twig
Browser->>API: POST /api/bulletins/read-all {ids}
API->>DB: INSERT/UPSERT bulletin_reads
DB-->>API: success
API-->>Browser: { success: true }
end
sequenceDiagram
participant Admin as Admin Browser
participant Server as App Server
participant API as Admin API
participant DB as Database
Admin->>Server: GET /admin/bulletins
Server->>API: GET /admin/api/bulletins
API->>DB: SELECT * FROM bulletins
DB-->>API: bulletins[]
API-->>Server: { success: true, bulletins }
Server-->>Admin: render admin/bulletins.twig
Admin->>Server: POST /admin/api/bulletins {data}
Server->>DB: INSERT/UPDATE bulletin
DB-->>Server: id/ok
Server-->>Admin: { success: true }
sequenceDiagram
participant TelnetUser as Telnet User
participant Session as BbsSession
participant Handler as BulletinsHandler
participant API as Bulletin API
participant DB as Database
TelnetUser->>Session: Login
Session->>Handler: showUnread(session)
Handler->>API: GET /api/bulletins?unread=1
API->>DB: SELECT unread active
DB-->>API: bulletins[]
API-->>Handler: bulletins
Handler->>TelnetUser: Render paged bulletins (plain/ANSI)
TelnetUser->>Handler: Next/Skip -> Handler posts read-all
Handler->>API: POST /api/bulletins/read-all { ids }
API->>DB: INSERT/UPSERT bulletin_reads
DB-->>API: success
Handler-->>TelnetUser: Return to menu
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
src/BbsConfig.php (1)
148-158: 💤 Low valueConsider adding phpDoc blocks for the new methods.
The implementation is correct. For consistency with the codebase conventions, consider adding brief phpDoc comments.
📝 Suggested phpDoc blocks
+ /** + * Return the configured bulletin display mode. + * + * `@return` string 'once' or 'always' + */ public static function getBulletinDisplayMode(): string { self::load(); $mode = strtolower(trim((string)(self::$config['bulletin_display_mode'] ?? 'once'))); return in_array($mode, ['once', 'always'], true) ? $mode : 'once'; } + /** + * Check if bulletins should be shown at every login session start. + * + * `@return` bool + */ public static function shouldAlwaysDisplayBulletins(): bool { return self::getBulletinDisplayMode() === 'always'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BbsConfig.php` around lines 148 - 158, Add phpDoc comments for the new methods to match project conventions: add a brief description and return type annotations for getBulletinDisplayMode() and shouldAlwaysDisplayBulletins(), e.g., document that getBulletinDisplayMode() returns the normalized bulletin display mode string ('once'|'always') and that shouldAlwaysDisplayBulletins() returns a bool indicating whether the mode is 'always'; place the docblocks immediately above the respective method declarations (getBulletinDisplayMode and shouldAlwaysDisplayBulletins).src/BulletinManager.php (1)
14-17: ⚡ Quick winMissing Logger property
BulletinManagerhas a constructor but noLoggerproperty, violating the coding guideline for allsrc/**/*.phpclasses.♻️ Proposed fix
class BulletinManager { private PDO $db; + private \BinktermPHP\Binkp\Logger $logger; public function __construct(?PDO $db = null) { $this->db = $db ?? Database::getInstance()->getPdo(); + $this->logger = new \BinktermPHP\Binkp\Logger('logs/server.log'); }As per coding guidelines: "In a class with a constructor, inject or create a Logger property with
new \BinktermPHP\Binkp\Logger()using the appropriate log file path and level."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BulletinManager.php` around lines 14 - 17, BulletinManager's constructor sets $this->db but doesn't create/inject the required Logger property; update the class so the constructor (public function __construct(?PDO $db = null)) also initializes a Logger instance (new \BinktermPHP\Binkp\Logger()) assigned to a property like $this->logger, using the appropriate log file path and level per project conventions, or accept a Logger via constructor injection if preferred; ensure all methods in BulletinManager use $this->logger instead of any global logger and add the private Logger property declaration to the class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/i18n/fr/common.php`:
- Around line 3607-3610: Replace the ASCII-fallback French strings for the
bulletin display keys with proper literal accented characters and correct
apostrophes: update 'ui.admin.bbs_settings.features.bulletin_display_mode',
'ui.admin.bbs_settings.features.bulletin_display_once',
'ui.admin.bbs_settings.features.bulletin_display_always', and
'ui.admin.bbs_settings.features.bulletin_display_help' to use proper French
accents (e.g., "Mode d'affichage des bulletins", "Afficher une fois", "Toujours
afficher", and "Choisissez si les bulletins de connexion sont affichés jusqu'à
leur lecture, ou une fois au début de chaque session.") ensuring you use literal
characters like é, à, and proper apostrophes instead of ASCII fallbacks.
- Around line 4562-4594: Update the French bulletin localization entries to use
proper French accents and typographic apostrophes: replace ASCII-only text in
keys like ui.bulletins.prev, ui.bulletins.done, ui.bulletins.no_new,
ui.dashboard.bulletins.unread, ui.admin.bulletins.create,
ui.admin.bulletins.edit, ui.admin.bulletins.delete_confirm,
ui.admin.bulletins.active_until, ui.admin.bulletins.preview,
ui.admin.bulletins.none, ui.admin.bulletins.load_failed,
ui.admin.bulletins.save_failed, ui.admin.bulletins.saved_success,
ui.admin.bulletins.deleted_success, ui.admin.bulletins.reordered_success (and
any other bulletin keys in the same block) to use literal characters (e.g.,
Précédent, Terminé, Aucun nouveau bulletin → Aucun nouveau bulletin or Aucun
nouveau bulletin as appropriate, Supprimer ce bulletin ? → Supprimer ce
bulletin ?, Actif jusqu’à, Aperçu, Aucun bulletin., Impossible de charger les
bulletins., Impossible d’enregistrer le bulletin., Bulletin enregistré.,
Bulletin supprimé., Bulletins réordonnés.) ensuring correct accents and
typographic apostrophes throughout.
In `@config/i18n/it/common.php`:
- Around line 1788-1791: Update the Italian strings for the bulletin display
labels: change the value of
'ui.admin.bbs_settings.features.bulletin_display_mode' from "Modalita
visualizzazione bollettini" to use the accented i.e. "Modalità visualizzazione
bollettini", and fix punctuation/diacritics in
'ui.admin.bbs_settings.features.bulletin_display_help' by replacing "finche"
with "finché" and "all inizio" with "all'inizio" so the sentence reads "Scegli
se i bollettini di accesso vengono mostrati finché letti, oppure una volta
all'inizio di ogni sessione." Ensure you use literal accented characters and the
correct apostrophe.
In `@routes/admin-routes.php`:
- Around line 1473-1493: The route handler registered with
SimpleRouter::put('/bulletins/{id}', function($id) { ... }) casts non-numeric
$id to 0 and can silently operate on invalid IDs; before calling (new
\BinktermPHP\BulletinManager())->update((int)$id, $payload) validate that $id is
a positive integer (e.g. use is_numeric/ctype_digit and intval > 0) and return
an apiError (400) if not, so only numeric IDs are accepted and you avoid
misleading success responses for invalid paths.
- Around line 1441-1449: The bulletin routes (e.g., the
SimpleRouter::get('/bulletins'...) handler using RouteHelper::requireAdmin() and
BulletinManager->getAllBulletins()) must be wrapped in try/catch blocks that
catch exceptions, log details via getServerLogger()->error(...), and return a
structured JSON error using apiError(error_code, message, status, extra) instead
of echoing raw messages or letting exceptions bubble; apply the same pattern to
the other bulletin handlers referenced (GET/DELETE/reorder) so all failure paths
call apiError(...) with a stable error_code and status and include any useful
extra info (but not raw stack traces) while using getServerLogger() for internal
logging.
- Around line 1461-1467: The code treats any non-exception return from
BulletinManager::create() as success even when it returns 0; update the
post-create handling to check the returned $id (from BulletinManager::create)
and if $id === 0 treat it as a failure: return a JSON response with 'success' =>
false and an appropriate 'message_code' (or error info) instead of the current
success payload; use a strict comparison ($id === 0) and keep the existing
try/catch flow so exceptions still return the error response.
In `@routes/api-routes.php`:
- Around line 764-771: The route closure registered with SimpleRouter::post for
'/bulletins/{id}/read' currently casts $id to (int) and can silently turn
invalid path values into 0 and return success; update the handler to validate
the incoming $id before calling BulletinManager::markRead: check that $id is a
positive integer (e.g., using ctype_digit or filter_var to ensure numeric and
(int)$id > 0), and if invalid return a structured 400 error via
apiError('invalid_bulletin_id', 'Invalid bulletin id', 400, ['id' => $id])
instead of echoing success; only call (new BulletinManager())->markRead($userId,
(int)$id) after validation and then return the success JSON.
- Around line 773-787: The handler registered with
SimpleRouter::post('/bulletins/read-all') accepts malformed JSON or non-array
payloads as success because $payload falls back to []; fix by reading raw input,
json_decoding and validating errors: if json_decode returns null and
json_last_error() !== JSON_ERROR_NONE (or if decoded value is not an array) call
apiError('errors.bulletins.invalid_ids',
apiLocalizedText('errors.bulletins.invalid_ids', 'Invalid bulletin list.',
$user), 400, ['error' => json_last_error_msg()]); ensure you still obtain $user
via RouteHelper::requireAuth() and pass a validated $ids array into (new
BulletinManager())->markAllRead($userId, $ids) only when validation passes.
In `@routes/web-routes.php`:
- Around line 232-245: Wrap the bulletin-loading and render logic in a try/catch
around the BulletinManager calls inside the SimpleRouter::get('/bulletins')
handler: call RouteHelper::requireAuth() as before, instantiate BulletinManager,
then if BbsConfig::shouldAlwaysDisplayBulletins() compute $active =
$manager->getActiveBulletins($userId) once and reuse it for both 'bulletins' and
'unread_bulletins'; otherwise call getActiveBulletins($userId) for 'bulletins'
and getUnreadBulletins($userId) for 'unread_bulletins'. In the catch block catch
exceptions from BulletinManager, log or handle the error and render an
appropriate error/empty response via Template->renderResponse('bulletins.twig',
...) so DB failures don't throw uncaught exceptions.
In `@src/BulletinManager.php`:
- Around line 273-281: normalizeDateTime currently lets new
\DateTimeImmutable($value) throw and bubble up; update normalizeDateTime to
detect/handle invalid input (use \DateTimeImmutable::createFromFormat or wrap
new \DateTimeImmutable in try/catch) and, on parse failure, throw a specific
validation exception (e.g. \InvalidArgumentException or your app's
ValidationException) with a clear message including the offending $value so
sanitizeData and the create/update flows can return a user-friendly validation
error instead of an HTTP 500; keep the return type of normalizeDateTime as
?string when valid and ensure callers (sanitizeData, create, update) treat the
thrown validation exception appropriately.
In `@telnet/src/BulletinsHandler.php`:
- Around line 179-186: nextActiveSgr currently ignores the accumulated
$activeSgr and returns only $sgr, losing prior SGR state across wrap boundaries;
change it to preserve and merge the accumulated style: if $sgr is a reset
sequence (matches the reset regex) return an empty string, otherwise return a
combined SGR that keeps previous state (use $activeSgr when present and
append/merge the new $sgr) so wrapAnsiSgrLine receives the full compound SGR
state for continuation lines (update nextActiveSgr to reference $activeSgr and
return the merged/concatenated SGR accordingly).
In `@templates/admin/bulletins.twig`:
- Around line 203-209: The admin datetime round-trip loses timezone because
toLocalInputValue renders local values but collectPayload sends the bare
"YYYY-MM-DDTHH:MM" string; update collectPayload to convert the input string
into a JS Date (which treats that string as local) and send its UTC ISO form
(Date(value).toISOString()) for fields like active_from/active_until so the
server receives a Z-suffixed UTC timestamp that PHP's normalizeDateTime can
parse correctly; leave toLocalInputValue unchanged.
In `@templates/bulletins.twig`:
- Line 58: The template currently outputs unescaped JSON into a script tag via
"const bulletins = {{ unread_bulletins|json_encode|raw }}", which allows literal
</script> from bulletin content to break out; change the json_encode call to
pass JSON_HEX_TAG (and preferably JSON_HEX_APOS|JSON_HEX_QUOT|JSON_HEX_AMP) so
angle brackets and other dangerous characters are hex-escaped (e.g., {{
unread_bulletins|json_encode(constant('JSON_HEX_TAG') b-or
constant('JSON_HEX_AMP') b-or constant('JSON_HEX_APOS') b-or
constant('JSON_HEX_QUOT'))|raw }}), leaving the raw filter only if you need
unescaped output after safe encoding.
---
Nitpick comments:
In `@src/BbsConfig.php`:
- Around line 148-158: Add phpDoc comments for the new methods to match project
conventions: add a brief description and return type annotations for
getBulletinDisplayMode() and shouldAlwaysDisplayBulletins(), e.g., document that
getBulletinDisplayMode() returns the normalized bulletin display mode string
('once'|'always') and that shouldAlwaysDisplayBulletins() returns a bool
indicating whether the mode is 'always'; place the docblocks immediately above
the respective method declarations (getBulletinDisplayMode and
shouldAlwaysDisplayBulletins).
In `@src/BulletinManager.php`:
- Around line 14-17: BulletinManager's constructor sets $this->db but doesn't
create/inject the required Logger property; update the class so the constructor
(public function __construct(?PDO $db = null)) also initializes a Logger
instance (new \BinktermPHP\Binkp\Logger()) assigned to a property like
$this->logger, using the appropriate log file path and level per project
conventions, or accept a Logger via constructor injection if preferred; ensure
all methods in BulletinManager use $this->logger instead of any global logger
and add the private Logger property declaration to the class.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d269c50-80d8-4818-9ebf-daec7aae2b1b
📒 Files selected for processing (28)
config/bbs.json.exampleconfig/i18n/en/common.phpconfig/i18n/es/common.phpconfig/i18n/fr/common.phpconfig/i18n/it/common.phpdatabase/migrations/v1.11.0.88_bulletins.sqldocs/UPGRADING_1.9.5.mdpublic_html/js/markdown-editor.jspublic_html/sw.jsroutes/admin-routes.phproutes/api-routes.phproutes/web-routes.phpsrc/BbsConfig.phpsrc/BulletinManager.phpsrc/DashboardCardRegistry.phpsrc/Template.phpssh/ssh_daemon.phptelnet/src/BbsSession.phptelnet/src/BulletinsHandler.phptelnet/telnet_daemon.phptemplates/admin/bbs_settings.twigtemplates/admin/bulletins.twigtemplates/base.twigtemplates/bulletins.twigtemplates/compose.twigtemplates/dashboard.twigtemplates/shells/bbs-menu/base.twigtemplates/shells/web/base.twig
Comment on lines
+3607
to
+3610
| 'ui.admin.bbs_settings.features.bulletin_display_mode' => 'Mode d affichage des bulletins', | ||
| 'ui.admin.bbs_settings.features.bulletin_display_once' => 'Afficher une fois', | ||
| 'ui.admin.bbs_settings.features.bulletin_display_always' => 'Toujours afficher', | ||
| 'ui.admin.bbs_settings.features.bulletin_display_help' => 'Choisissez si les bulletins de connexion sont affiches jusqu a leur lecture, ou une fois au debut de chaque session.', |
There was a problem hiding this comment.
Use literal accented French characters in new bulletin display strings.
These new values use ASCII fallbacks instead of proper French characters (e.g., d affichage, jusqu a, debut). Please switch to literal accents/apostrophes to match catalog standards.
Proposed fix
- 'ui.admin.bbs_settings.features.bulletin_display_mode' => 'Mode d affichage des bulletins',
+ 'ui.admin.bbs_settings.features.bulletin_display_mode' => 'Mode d\'affichage des bulletins',
'ui.admin.bbs_settings.features.bulletin_display_once' => 'Afficher une fois',
'ui.admin.bbs_settings.features.bulletin_display_always' => 'Toujours afficher',
- 'ui.admin.bbs_settings.features.bulletin_display_help' => 'Choisissez si les bulletins de connexion sont affiches jusqu a leur lecture, ou une fois au debut de chaque session.',
+ 'ui.admin.bbs_settings.features.bulletin_display_help' => 'Choisissez si les bulletins de connexion sont affichés jusqu\'à leur lecture, ou une fois au début de chaque session.',As per coding guidelines: "When editing French or other accented catalogs, use literal characters (e.g., 'é', 'à') only."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/i18n/fr/common.php` around lines 3607 - 3610, Replace the
ASCII-fallback French strings for the bulletin display keys with proper literal
accented characters and correct apostrophes: update
'ui.admin.bbs_settings.features.bulletin_display_mode',
'ui.admin.bbs_settings.features.bulletin_display_once',
'ui.admin.bbs_settings.features.bulletin_display_always', and
'ui.admin.bbs_settings.features.bulletin_display_help' to use proper French
accents (e.g., "Mode d'affichage des bulletins", "Afficher une fois", "Toujours
afficher", and "Choisissez si les bulletins de connexion sont affichés jusqu'à
leur lecture, ou une fois au début de chaque session.") ensuring you use literal
characters like é, à, and proper apostrophes instead of ASCII fallbacks.
Comment on lines
+1788
to
+1791
| 'ui.admin.bbs_settings.features.bulletin_display_mode' => 'Modalita visualizzazione bollettini', | ||
| 'ui.admin.bbs_settings.features.bulletin_display_once' => 'Mostra una volta', | ||
| 'ui.admin.bbs_settings.features.bulletin_display_always' => 'Mostra sempre', | ||
| 'ui.admin.bbs_settings.features.bulletin_display_help' => 'Scegli se i bollettini di accesso vengono mostrati finche letti, oppure una volta all inizio di ogni sessione.', |
There was a problem hiding this comment.
Fix Italian diacritics/apostrophe in bulletin display labels
Line 1788 and Line 1791 have misspellings (Modalita, finche, all inizio) that should use proper Italian characters/apostrophe.
✏️ Suggested fix
- 'ui.admin.bbs_settings.features.bulletin_display_mode' => 'Modalita visualizzazione bollettini',
+ 'ui.admin.bbs_settings.features.bulletin_display_mode' => 'Modalità visualizzazione bollettini',
@@
- 'ui.admin.bbs_settings.features.bulletin_display_help' => 'Scegli se i bollettini di accesso vengono mostrati finche letti, oppure una volta all inizio di ogni sessione.',
+ 'ui.admin.bbs_settings.features.bulletin_display_help' => 'Scegli se i bollettini di accesso vengono mostrati finché letti, oppure una volta all’inizio di ogni sessione.',As per coding guidelines, "When editing French or other accented catalogs, use literal characters (e.g., 'é', 'à') only."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 'ui.admin.bbs_settings.features.bulletin_display_mode' => 'Modalita visualizzazione bollettini', | |
| 'ui.admin.bbs_settings.features.bulletin_display_once' => 'Mostra una volta', | |
| 'ui.admin.bbs_settings.features.bulletin_display_always' => 'Mostra sempre', | |
| 'ui.admin.bbs_settings.features.bulletin_display_help' => 'Scegli se i bollettini di accesso vengono mostrati finche letti, oppure una volta all inizio di ogni sessione.', | |
| 'ui.admin.bbs_settings.features.bulletin_display_mode' => 'Modalità visualizzazione bollettini', | |
| 'ui.admin.bbs_settings.features.bulletin_display_once' => 'Mostra una volta', | |
| 'ui.admin.bbs_settings.features.bulletin_display_always' => 'Mostra sempre', | |
| 'ui.admin.bbs_settings.features.bulletin_display_help' => 'Scegli se i bollettini di accesso vengono mostrati finché letti, oppure una volta all'inizio di ogni sessione.', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/i18n/it/common.php` around lines 1788 - 1791, Update the Italian
strings for the bulletin display labels: change the value of
'ui.admin.bbs_settings.features.bulletin_display_mode' from "Modalita
visualizzazione bollettini" to use the accented i.e. "Modalità visualizzazione
bollettini", and fix punctuation/diacritics in
'ui.admin.bbs_settings.features.bulletin_display_help' by replacing "finche"
with "finché" and "all inizio" with "all'inizio" so the sentence reads "Scegli
se i bollettini di accesso vengono mostrati finché letti, oppure una volta
all'inizio di ogni sessione." Ensure you use literal accented characters and the
correct apostrophe.
Comment on lines
+1441
to
+1449
| SimpleRouter::get('/bulletins', function() { | ||
| RouteHelper::requireAdmin(); | ||
|
|
||
| header('Content-Type: application/json'); | ||
| echo json_encode([ | ||
| 'success' => true, | ||
| 'bulletins' => (new \BinktermPHP\BulletinManager())->getAllBulletins(), | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Unify bulletin API failure handling to structured, non-leaky errors.
Line 1469 and Line 1490 return raw exception messages to clients, while Line 1441-Line 1449 / Line 1494-Line 1521 have no exception guard at all. That can leak internals and break the JSON error contract under runtime/database failures.
💡 Suggested direction
- } catch (\Throwable $e) {
- apiError('errors.admin.bulletins.save_failed', $e->getMessage(), 400);
+ } catch (\Throwable $e) {
+ getServerLogger()->error('Bulletin save failed: ' . $e->getMessage());
+ apiError(
+ 'errors.admin.bulletins.save_failed',
+ apiLocalizedText('errors.admin.bulletins.save_failed', 'Failed to save bulletin.'),
+ 400
+ );
}Apply the same pattern to GET/DELETE/reorder so all failures return consistent apiError(...) payloads.
As per coding guidelines, route files should use the getServerLogger() helper and API responses should use structured errors via apiError(error_code, message, status, extra) with error_code and error.
Also applies to: 1468-1470, 1489-1490, 1494-1503, 1505-1521
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@routes/admin-routes.php` around lines 1441 - 1449, The bulletin routes (e.g.,
the SimpleRouter::get('/bulletins'...) handler using RouteHelper::requireAdmin()
and BulletinManager->getAllBulletins()) must be wrapped in try/catch blocks that
catch exceptions, log details via getServerLogger()->error(...), and return a
structured JSON error using apiError(error_code, message, status, extra) instead
of echoing raw messages or letting exceptions bubble; apply the same pattern to
the other bulletin handlers referenced (GET/DELETE/reorder) so all failure paths
call apiError(...) with a stable error_code and status and include any useful
extra info (but not raw stack traces) while using getServerLogger() for internal
logging.
Comment on lines
+1461
to
+1467
| try { | ||
| $id = (new \BinktermPHP\BulletinManager())->create($payload, (int)($user['user_id'] ?? $user['id'] ?? 0)); | ||
| echo json_encode([ | ||
| 'success' => true, | ||
| 'id' => $id, | ||
| 'message_code' => 'ui.admin.bulletins.saved_success', | ||
| ]); |
There was a problem hiding this comment.
Handle create() returning 0 as failure instead of success.
At Line 1462, BulletinManager::create() can return 0 (see src/BulletinManager.php Line 140-Line 161). Returning success: true with id: 0 creates a false-positive save result.
💡 Suggested fix
try {
$id = (new \BinktermPHP\BulletinManager())->create($payload, (int)($user['user_id'] ?? $user['id'] ?? 0));
+ if ($id <= 0) {
+ apiError(
+ 'errors.admin.bulletins.save_failed',
+ apiLocalizedText('errors.admin.bulletins.save_failed', 'Failed to save bulletin.'),
+ 500
+ );
+ return;
+ }
echo json_encode([
'success' => true,
'id' => $id,
'message_code' => 'ui.admin.bulletins.saved_success',
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| $id = (new \BinktermPHP\BulletinManager())->create($payload, (int)($user['user_id'] ?? $user['id'] ?? 0)); | |
| echo json_encode([ | |
| 'success' => true, | |
| 'id' => $id, | |
| 'message_code' => 'ui.admin.bulletins.saved_success', | |
| ]); | |
| try { | |
| $id = (new \BinktermPHP\BulletinManager())->create($payload, (int)($user['user_id'] ?? $user['id'] ?? 0)); | |
| if ($id <= 0) { | |
| apiError( | |
| 'errors.admin.bulletins.save_failed', | |
| apiLocalizedText('errors.admin.bulletins.save_failed', 'Failed to save bulletin.'), | |
| 500 | |
| ); | |
| return; | |
| } | |
| echo json_encode([ | |
| 'success' => true, | |
| 'id' => $id, | |
| 'message_code' => 'ui.admin.bulletins.saved_success', | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@routes/admin-routes.php` around lines 1461 - 1467, The code treats any
non-exception return from BulletinManager::create() as success even when it
returns 0; update the post-create handling to check the returned $id (from
BulletinManager::create) and if $id === 0 treat it as a failure: return a JSON
response with 'success' => false and an appropriate 'message_code' (or error
info) instead of the current success payload; use a strict comparison ($id ===
0) and keep the existing try/catch flow so exceptions still return the error
response.
Comment on lines
+232
to
+245
| SimpleRouter::get('/bulletins', function() { | ||
| $user = RouteHelper::requireAuth(); | ||
| $userId = (int)($user['user_id'] ?? $user['id'] ?? 0); | ||
| $manager = new BulletinManager(); | ||
| unset($_SESSION['show_login_bulletins_for_session']); | ||
|
|
||
| $template = new Template(); | ||
| $template->renderResponse('bulletins.twig', [ | ||
| 'bulletins' => $manager->getActiveBulletins($userId), | ||
| 'unread_bulletins' => BbsConfig::shouldAlwaysDisplayBulletins() | ||
| ? $manager->getActiveBulletins($userId) | ||
| : $manager->getUnreadBulletins($userId), | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
/bulletins route: missing error handling and duplicate DB query in "always display" mode
No try/catch wraps the BulletinManager calls here, unlike the dashboard at lines 147–151. A database failure will surface as an unhandled exception. Additionally, when shouldAlwaysDisplayBulletins() is true, getActiveBulletins($userId) is executed twice for the same result (lines 240 and 242).
🛡️ Proposed fix
SimpleRouter::get('/bulletins', function() {
$user = RouteHelper::requireAuth();
$userId = (int)($user['user_id'] ?? $user['id'] ?? 0);
- $manager = new BulletinManager();
unset($_SESSION['show_login_bulletins_for_session']);
- $template = new Template();
- $template->renderResponse('bulletins.twig', [
- 'bulletins' => $manager->getActiveBulletins($userId),
- 'unread_bulletins' => BbsConfig::shouldAlwaysDisplayBulletins()
- ? $manager->getActiveBulletins($userId)
- : $manager->getUnreadBulletins($userId),
- ]);
+ $activeBulletins = [];
+ $unreadBulletins = [];
+ try {
+ $manager = new BulletinManager();
+ $activeBulletins = $manager->getActiveBulletins($userId);
+ $unreadBulletins = BbsConfig::shouldAlwaysDisplayBulletins()
+ ? $activeBulletins
+ : $manager->getUnreadBulletins($userId);
+ } catch (\Throwable $e) {
+ getServerLogger()->warning("Bulletins page load failed: " . $e->getMessage());
+ }
+
+ $template = new Template();
+ $template->renderResponse('bulletins.twig', [
+ 'bulletins' => $activeBulletins,
+ 'unread_bulletins' => $unreadBulletins,
+ ]);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@routes/web-routes.php` around lines 232 - 245, Wrap the bulletin-loading and
render logic in a try/catch around the BulletinManager calls inside the
SimpleRouter::get('/bulletins') handler: call RouteHelper::requireAuth() as
before, instantiate BulletinManager, then if
BbsConfig::shouldAlwaysDisplayBulletins() compute $active =
$manager->getActiveBulletins($userId) once and reuse it for both 'bulletins' and
'unread_bulletins'; otherwise call getActiveBulletins($userId) for 'bulletins'
and getUnreadBulletins($userId) for 'unread_bulletins'. In the catch block catch
exceptions from BulletinManager, log or handle the error and render an
appropriate error/empty response via Template->renderResponse('bulletins.twig',
...) so DB failures don't throw uncaught exceptions.
Comment on lines
+273
to
+281
| private function normalizeDateTime($value): ?string | ||
| { | ||
| $value = trim((string)($value ?? '')); | ||
| if ($value === '') { | ||
| return null; | ||
| } | ||
|
|
||
| return (new \DateTimeImmutable($value))->format('Y-m-d H:i:sP'); | ||
| } |
There was a problem hiding this comment.
normalizeDateTime throws an unhandled exception on invalid date input, producing HTTP 500
new \DateTimeImmutable($value) throws \Exception for any malformed date string (e.g. "not-a-date", "2026-13-01"). This propagates uncaught through sanitizeData and then through create/update, resulting in an unhandled server error for the admin rather than a clean validation message.
🐛 Proposed fix
private function normalizeDateTime($value): ?string
{
$value = trim((string)($value ?? ''));
if ($value === '') {
return null;
}
- return (new \DateTimeImmutable($value))->format('Y-m-d H:i:sP');
+ try {
+ return (new \DateTimeImmutable($value))->format('Y-m-d H:i:sP');
+ } catch (\Exception $e) {
+ throw new \InvalidArgumentException('Invalid date/time value: ' . $value);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private function normalizeDateTime($value): ?string | |
| { | |
| $value = trim((string)($value ?? '')); | |
| if ($value === '') { | |
| return null; | |
| } | |
| return (new \DateTimeImmutable($value))->format('Y-m-d H:i:sP'); | |
| } | |
| private function normalizeDateTime($value): ?string | |
| { | |
| $value = trim((string)($value ?? '')); | |
| if ($value === '') { | |
| return null; | |
| } | |
| try { | |
| return (new \DateTimeImmutable($value))->format('Y-m-d H:i:sP'); | |
| } catch (\Exception $e) { | |
| throw new \InvalidArgumentException('Invalid date/time value: ' . $value); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/BulletinManager.php` around lines 273 - 281, normalizeDateTime currently
lets new \DateTimeImmutable($value) throw and bubble up; update
normalizeDateTime to detect/handle invalid input (use
\DateTimeImmutable::createFromFormat or wrap new \DateTimeImmutable in
try/catch) and, on parse failure, throw a specific validation exception (e.g.
\InvalidArgumentException or your app's ValidationException) with a clear
message including the offending $value so sanitizeData and the create/update
flows can return a user-friendly validation error instead of an HTTP 500; keep
the return type of normalizeDateTime as ?string when valid and ensure callers
(sanitizeData, create, update) treat the thrown validation exception
appropriately.
Comment on lines
+179
to
+186
| private function nextActiveSgr(string $activeSgr, string $sgr): string | ||
| { | ||
| if (preg_match('/\033\[(?:0|39|49)?m/', $sgr)) { | ||
| return ''; | ||
| } | ||
|
|
||
| return $sgr; | ||
| } |
There was a problem hiding this comment.
nextActiveSgr ignores $activeSgr — accumulated SGR state is lost at wrap boundaries
PHPMD confirms $activeSgr is never read. The function returns the incoming $sgr as-is for non-reset sequences, completely discarding the previous accumulated style. At every wrap boundary in wrapAnsiSgrLine, the continuation segment is prefixed with only the last single SGR sequence, not the compound state. For example, text styled bold (\033[1m) then coloured (\033[32m) would have bold silently dropped in all wrapped continuation lines.
🐛 Proposed fix
private function nextActiveSgr(string $activeSgr, string $sgr): string
{
if (preg_match('/\033\[(?:0|39|49)?m/', $sgr)) {
return '';
}
- return $sgr;
+ // Accumulate: carry prior state into the new segment, then apply the new code.
+ // For compound attribute sequences emitted as a single escape (e.g. \033[1;32m),
+ // this is a no-op beyond the single call, so accumulation only matters when
+ // callers apply bold, colour, etc. with separate escape sequences.
+ return $activeSgr . $sgr;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private function nextActiveSgr(string $activeSgr, string $sgr): string | |
| { | |
| if (preg_match('/\033\[(?:0|39|49)?m/', $sgr)) { | |
| return ''; | |
| } | |
| return $sgr; | |
| } | |
| private function nextActiveSgr(string $activeSgr, string $sgr): string | |
| { | |
| if (preg_match('/\033\[(?:0|39|49)?m/', $sgr)) { | |
| return ''; | |
| } | |
| // Accumulate: carry prior state into the new segment, then apply the new code. | |
| // For compound attribute sequences emitted as a single escape (e.g. \033[1;32m), | |
| // this is a no-op beyond the single call, so accumulation only matters when | |
| // callers apply bold, colour, etc. with separate escape sequences. | |
| return $activeSgr . $sgr; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 179-179: Avoid unused parameters such as '$activeSgr'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@telnet/src/BulletinsHandler.php` around lines 179 - 186, nextActiveSgr
currently ignores the accumulated $activeSgr and returns only $sgr, losing prior
SGR state across wrap boundaries; change it to preserve and merge the
accumulated style: if $sgr is a reset sequence (matches the reset regex) return
an empty string, otherwise return a combined SGR that keeps previous state (use
$activeSgr when present and append/merge the new $sgr) so wrapAnsiSgrLine
receives the full compound SGR state for continuation lines (update
nextActiveSgr to reference $activeSgr and return the merged/concatenated SGR
accordingly).
Comment on lines
+203
to
+209
| function toLocalInputValue(value) { | ||
| if (!value) return ''; | ||
| const date = new Date(value); | ||
| if (Number.isNaN(date.getTime())) return ''; | ||
| const pad = n => String(n).padStart(2, '0'); | ||
| return `${date.getFullYear()}-${pad(date.getMonth() + 1)}-${pad(date.getDate())}T${pad(date.getHours())}:${pad(date.getMinutes())}`; | ||
| } |
There was a problem hiding this comment.
Datetime round-trip discards timezone: admin stores wrong UTC values
toLocalInputValue (lines 203–209) correctly renders stored UTC timestamps in the admin's local timezone using date.getHours() etc. However, collectPayload (lines 306–307) reads the datetime-local input back as a bare string like "2026-05-01T12:00" — with no timezone suffix — and sends it to the server. PHP's new \DateTimeImmutable("2026-05-01T12:00") then interprets this as server time (typically UTC). An admin in UTC−5 who enters "noon" as the active_from time will have the bulletin become active at noon UTC — 5 hours earlier than intended.
Fix: convert the local datetime back to a UTC ISO string before submission. new Date("2026-05-01T12:00") is treated as local time by all modern browsers per the HTML spec, so .toISOString() yields the correct UTC equivalent.
🐛 Proposed fix
+function localInputToUtcIso(value) {
+ if (!value) return null;
+ const dt = new Date(value); // browser treats no-tz datetime-local value as local time
+ return isNaN(dt.getTime()) ? null : dt.toISOString();
+}
+
function collectPayload() {
syncBulletinMarkdownEditor();
return {
title: document.getElementById('bulletinTitle').value.trim(),
body: document.getElementById('bulletinBody').value,
format: document.getElementById('bulletinFormat').value,
sort_order: parseInt(document.getElementById('bulletinSort').value || '0', 10),
is_active: document.getElementById('bulletinActive').checked,
- active_from: document.getElementById('bulletinFrom').value || null,
- active_until: document.getElementById('bulletinUntil').value || null
+ active_from: localInputToUtcIso(document.getElementById('bulletinFrom').value),
+ active_until: localInputToUtcIso(document.getElementById('bulletinUntil').value)
};
}normalizeDateTime on the PHP side already handles ISO 8601 strings including the Z suffix, so no server-side changes are needed.
Also applies to: 298-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/admin/bulletins.twig` around lines 203 - 209, The admin datetime
round-trip loses timezone because toLocalInputValue renders local values but
collectPayload sends the bare "YYYY-MM-DDTHH:MM" string; update collectPayload
to convert the input string into a JS Date (which treats that string as local)
and send its UTC ISO form (Date(value).toISOString()) for fields like
active_from/active_until so the server receives a Z-suffixed UTC timestamp that
PHP's normalizeDateTime can parse correctly; leave toLocalInputValue unchanged.
| {% block scripts %} | ||
| <script src="/js/ansisys.js"></script> | ||
| <script> | ||
| const bulletins = {{ unread_bulletins|json_encode|raw }}; |
There was a problem hiding this comment.
json_encode|raw without JSON_HEX_TAG can prematurely close the <script> block
PHP's json_encode does not escape < and > by default. A bulletin body containing the literal string </script> (which a sysop could write accidentally or in ANSI art content) will close the surrounding <script> tag in the browser, causing a parse error or enabling further injection.
🛡️ Proposed fix
-const bulletins = {{ unread_bulletins|json_encode|raw }};
+const bulletins = {{ unread_bulletins|json_encode(constant('JSON_HEX_TAG'))|raw }};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const bulletins = {{ unread_bulletins|json_encode|raw }}; | |
| const bulletins = {{ unread_bulletins|json_encode(constant('JSON_HEX_TAG'))|raw }}; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/bulletins.twig` at line 58, The template currently outputs
unescaped JSON into a script tag via "const bulletins = {{
unread_bulletins|json_encode|raw }}", which allows literal </script> from
bulletin content to break out; change the json_encode call to pass JSON_HEX_TAG
(and preferably JSON_HEX_APOS|JSON_HEX_QUOT|JSON_HEX_AMP) so angle brackets and
other dangerous characters are hex-escaped (e.g., {{
unread_bulletins|json_encode(constant('JSON_HEX_TAG') b-or
constant('JSON_HEX_AMP') b-or constant('JSON_HEX_APOS') b-or
constant('JSON_HEX_QUOT'))|raw }}), leaving the raw filter only if you need
unescaped output after safe encoding.
- Add fork-first setup (fork → clone fork → add upstream remote) - Change PR target branch from main to claudesbbs throughout - Explain claudesbbs is a staging branch deployed to Claude's BBS for live testing before merging into main for release - Correct migration directory from schema/ to database/migrations/ - Remove git tag instruction (maintainers handle tagging, not contributors) - Remove dangling Changelog Updates TOC entry (no project changelog exists) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@routes/web-routes.php`:
- Around line 237-241: The flag $showUnreadOnly can remain true even when
BbsConfig::shouldAlwaysDisplayBulletins() forces $displayBulletins to use
$activeBulletins; update the logic so $showUnreadOnly is cleared when
always-display is enabled (e.g. set $showUnreadOnly = !empty($_GET['unread']) &&
!BbsConfig::shouldAlwaysDisplayBulletins() before computing $displayBulletins)
and apply the same change to the similar block around line 247 so the UI
unread-mode flag always matches the actual $displayBulletins content.
In `@telnet/src/BbsSession.php`:
- Around line 2178-2181: The current wrapEditorLines usage and implementation is
byte-oriented and can split UTF-8 codepoints; update wrapEditorLines (and its
callers at the other noted blocks) to branch on the session encoding (e.g. an
existing UTF-8 flag like $this->utf8Mode or similar) and, when in UTF-8 mode,
perform wrapping and cursor math with multibyte/width-aware functions
(mb_strlen/mb_substr/mb_strrpos and mb_strwidth or equivalent
grapheme/width-safe routines) so you split on character/grapheme/column
boundaries rather than bytes; retain the existing byte-oriented code path for
CP437/ASCII sessions. Ensure returned $lines and adjusted $cursorRow/$cursorCol
are computed in the same unit (display columns/characters) used by the editor UI
so saved message bodies remain valid UTF-8.
In `@templates/settings.twig`:
- Line 47: The form with id "settingsForm" currently removes its loading state
when the fetch to "/api/user/settings" fails but does not surface the error or
re-enable actions; update the fetch error handling to set a visible error
message in the UI (e.g., insert text into a dedicated error container or render
an alert near the form), keep or restore an appropriate disabled state, and
enable the Save button with a tooltip or visible explanation so users know why
save is unavailable; locate the fetch logic that populates "settingsForm" and
the code that toggles the "settings-loading" / aria-busy attribute and modify
the failure branch to display the error and update the Save button state
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bbb07036-8744-4c77-aca2-f70f088d6987
📒 Files selected for processing (25)
CLAUDE.mdCONTRIBUTING.mdcomposer.jsonconfig/i18n/en/common.phpconfig/i18n/en/terminalserver.phpconfig/i18n/es/common.phpconfig/i18n/es/terminalserver.phpconfig/i18n/fr/common.phpconfig/i18n/fr/terminalserver.phpconfig/i18n/it/common.phpconfig/i18n/it/terminalserver.phpdocs/UPGRADING_1.9.5.mddocs/index.mdpublic_html/css/amber.csspublic_html/css/cyberpunk.csspublic_html/css/dark.csspublic_html/css/greenterm.csspublic_html/sw.jsroutes/web-routes.phpsrc/Version.phptelnet/src/BbsSession.phptelnet/src/TerminalBoxRenderer.phptemplates/admin/bulletins.twigtemplates/bulletins.twigtemplates/settings.twig
✅ Files skipped from review due to trivial changes (15)
- src/Version.php
- composer.json
- docs/index.md
- public_html/css/greenterm.css
- config/i18n/en/terminalserver.php
- config/i18n/es/terminalserver.php
- public_html/css/cyberpunk.css
- public_html/css/amber.css
- config/i18n/it/common.php
- CLAUDE.md
- config/i18n/fr/common.php
- docs/UPGRADING_1.9.5.md
- CONTRIBUTING.md
- config/i18n/fr/terminalserver.php
- config/i18n/en/common.php
🚧 Files skipped from review as they are similar to previous changes (3)
- templates/bulletins.twig
- templates/admin/bulletins.twig
- config/i18n/es/common.php
Comment on lines
+237
to
+241
| $showUnreadOnly = !empty($_GET['unread']); | ||
| $activeBulletins = $manager->getActiveBulletins($userId); | ||
| $displayBulletins = $showUnreadOnly && !BbsConfig::shouldAlwaysDisplayBulletins() | ||
| ? $manager->getUnreadBulletins($userId) | ||
| : $activeBulletins; |
There was a problem hiding this comment.
Keep unread-mode flag consistent with display mode.
At Line 237, show_unread_only can be true even when always-display mode forces display_bulletins to all active bulletins (Lines 239-241), which can desync UI state from actual content.
Suggested fix
- $showUnreadOnly = !empty($_GET['unread']);
+ $alwaysDisplay = BbsConfig::shouldAlwaysDisplayBulletins();
+ $showUnreadOnly = !empty($_GET['unread']) && !$alwaysDisplay;
$activeBulletins = $manager->getActiveBulletins($userId);
- $displayBulletins = $showUnreadOnly && !BbsConfig::shouldAlwaysDisplayBulletins()
+ $displayBulletins = $showUnreadOnly
? $manager->getUnreadBulletins($userId)
: $activeBulletins;Also applies to: 247-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@routes/web-routes.php` around lines 237 - 241, The flag $showUnreadOnly can
remain true even when BbsConfig::shouldAlwaysDisplayBulletins() forces
$displayBulletins to use $activeBulletins; update the logic so $showUnreadOnly
is cleared when always-display is enabled (e.g. set $showUnreadOnly =
!empty($_GET['unread']) && !BbsConfig::shouldAlwaysDisplayBulletins() before
computing $displayBulletins) and apply the same change to the similar block
around line 247 so the UI unread-mode flag always matches the actual
$displayBulletins content.
Comment on lines
+2178
to
+2181
| $editorWidth = max(10, $cols - 2); | ||
| $cursorRow = 0; | ||
| $cursorCol = 0; | ||
| [$lines, $cursorRow, $cursorCol] = $this->wrapEditorLines($lines, $cursorRow, $cursorCol, $editorWidth); |
There was a problem hiding this comment.
Make the editor wrap logic UTF-8-safe.
wrapEditorLines() is byte-oriented (strlen/substr/strrpos), but this session can run in UTF-8 mode. That means quoted or typed non-ASCII text can be split mid-codepoint during reflow, and because the wrapped buffer is what gets returned at the end, this can save invalid UTF-8 back into the message body. Please use width-aware multibyte wrapping for UTF-8 sessions and keep the current byte path only for CP437/ASCII.
Also applies to: 2252-2268, 2282-2330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@telnet/src/BbsSession.php` around lines 2178 - 2181, The current
wrapEditorLines usage and implementation is byte-oriented and can split UTF-8
codepoints; update wrapEditorLines (and its callers at the other noted blocks)
to branch on the session encoding (e.g. an existing UTF-8 flag like
$this->utf8Mode or similar) and, when in UTF-8 mode, perform wrapping and cursor
math with multibyte/width-aware functions (mb_strlen/mb_substr/mb_strrpos and
mb_strwidth or equivalent grapheme/width-safe routines) so you split on
character/grapheme/column boundaries rather than bytes; retain the existing
byte-oriented code path for CP437/ASCII sessions. Ensure returned $lines and
adjusted $cursorRow/$cursorCol are computed in the same unit (display
columns/characters) used by the editor UI so saved message bodies remain valid
UTF-8.
| {# Right content #} | ||
| <div class="col-md-9 col-lg-10"> | ||
| <form id="settingsForm"> | ||
| <form id="settingsForm" class="settings-loading" aria-busy="true"> |
There was a problem hiding this comment.
Surface the initial load failure in the page UI.
If /api/user/settings fails here, the form drops out of the loading state, keeps the hardcoded defaults on screen, and leaves the Save button disabled with no visible explanation. That makes the page look usable while blocking the task entirely.
Suggested fix
.fail(function() {
console.log(uiT('ui.settings.load_failed_console', 'Failed to load settings'));
+ $('#settingsStatus').show().html(`
+ <div class="alert alert-danger">
+ <i class="fas fa-exclamation-triangle me-2"></i>
+ ${escapeHtml(uiT('ui.settings.load_failed', 'Failed to load settings. Please reload and try again.'))}
+ </div>
+ `);
$('#settingsForm').attr('aria-busy', 'false');
setSettingsLoading('#settingsForm', false);
});Also applies to: 607-607, 795-799
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/settings.twig` at line 47, The form with id "settingsForm"
currently removes its loading state when the fetch to "/api/user/settings" fails
but does not surface the error or re-enable actions; update the fetch error
handling to set a visible error message in the UI (e.g., insert text into a
dedicated error container or render an alert near the form), keep or restore an
appropriate disabled state, and enable the Save button with a tooltip or visible
explanation so users know why save is unavailable; locate the fetch logic that
populates "settingsForm" and the code that toggles the "settings-loading" /
aria-busy attribute and modify the failure branch to display the error and
update the Save button state accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
CONTRIBUTING.md (1)
157-160: 💤 Low valueAdd language specifier to fenced code block.
The code block showing the migration filename format should have a language specifier for consistency and to satisfy markdownlint.
📝 Suggested fix
1. Create a timestamped migration file in `database/migrations/` following the naming convention: - ``` + ```text vYYYYMMDDHHMMSS_<description>.sql ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 157 - 160, The fenced code block showing the migration filename format in CONTRIBUTING.md lacks a language specifier which triggers markdownlint; update the triple-backtick fence for the migration example (the block containing "vYYYYMMDDHHMMSS_<description>.sql") to include a language tag (e.g., ```text) so the example is consistently highlighted and passes linting.CLAUDE.md (1)
127-127: 💤 Low valueMinor grammar: "upgrade related" → "upgrade-related".
Per static analysis hint, compound adjectives should be hyphenated.
📝 Suggested fix
-- Database migrations are handled through scripts/setup.php. setup.php will also call upgrade.php which handles other upgrade related tasks. +- Database migrations are handled through scripts/setup.php. setup.php will also call upgrade.php which handles other upgrade-related tasks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 127, Update the sentence in CLAUDE.md that reads "setup.php will also call upgrade.php which handles other upgrade related tasks" to hyphenate the compound adjective by changing "upgrade related" to "upgrade-related" so it reads "upgrade-related tasks"; ensure the rest of the sentence punctuation and casing remain unchanged.scripts/upgrade.php (1)
76-77: 💤 Low valueALTER TABLE runs unconditionally on every setup/upgrade.
The
ALTER TABLE ... ALTER COLUMN version TYPE VARCHAR(32)executes every timecreateMigrationsTable()is called, even when the column is already the correct size. While PostgreSQL handles this gracefully (no-op if already correct type), it adds unnecessary overhead.Consider making this conditional or removing it once deployments have been migrated:
💡 Optional: guard the ALTER
$this->db->exec(" CREATE TABLE IF NOT EXISTS database_migrations ( id SERIAL PRIMARY KEY, version VARCHAR(32) NOT NULL UNIQUE, description TEXT, executed_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, checksum VARCHAR(64) ) "); - $this->db->exec("ALTER TABLE database_migrations ALTER COLUMN version TYPE VARCHAR(32)"); + // Widen column for existing deployments with VARCHAR(20) + $stmt = $this->db->query(" + SELECT character_maximum_length + FROM information_schema.columns + WHERE table_name = 'database_migrations' AND column_name = 'version' + "); + $currentLength = $stmt->fetchColumn(); + if ($currentLength !== false && (int)$currentLength < 32) { + $this->db->exec("ALTER TABLE database_migrations ALTER COLUMN version TYPE VARCHAR(32)"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgrade.php` around lines 76 - 77, The ALTER runs unconditionally in createMigrationsTable(), causing unnecessary ALTER TABLE calls; modify createMigrationsTable() to first query the catalog (e.g., information_schema.columns or pg_catalog) for table 'database_migrations' and column 'version' and only execute $this->db->exec("ALTER TABLE database_migrations ALTER COLUMN version TYPE VARCHAR(32)") when the column type/character_maximum_length is not already VARCHAR(32) (or when data_type/character_maximum_length != 'character varying'/'32'); alternatively remove the ALTER entirely after rolling out the migration in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CLAUDE.md`:
- Line 127: Update the sentence in CLAUDE.md that reads "setup.php will also
call upgrade.php which handles other upgrade related tasks" to hyphenate the
compound adjective by changing "upgrade related" to "upgrade-related" so it
reads "upgrade-related tasks"; ensure the rest of the sentence punctuation and
casing remain unchanged.
In `@CONTRIBUTING.md`:
- Around line 157-160: The fenced code block showing the migration filename
format in CONTRIBUTING.md lacks a language specifier which triggers
markdownlint; update the triple-backtick fence for the migration example (the
block containing "vYYYYMMDDHHMMSS_<description>.sql") to include a language tag
(e.g., ```text) so the example is consistently highlighted and passes linting.
In `@scripts/upgrade.php`:
- Around line 76-77: The ALTER runs unconditionally in createMigrationsTable(),
causing unnecessary ALTER TABLE calls; modify createMigrationsTable() to first
query the catalog (e.g., information_schema.columns or pg_catalog) for table
'database_migrations' and column 'version' and only execute
$this->db->exec("ALTER TABLE database_migrations ALTER COLUMN version TYPE
VARCHAR(32)") when the column type/character_maximum_length is not already
VARCHAR(32) (or when data_type/character_maximum_length != 'character
varying'/'32'); alternatively remove the ALTER entirely after rolling out the
migration in production.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f98fcb6f-b622-46d6-908a-28681b96d2c9
📒 Files selected for processing (14)
CLAUDE.mdCONTRIBUTING.mdREADME.mddocs/DEVELOPER_GUIDE.mddocs/UPGRADING_1.9.5.mddocs/proposals/CWN_Proposal.mdpublic_html/webdoors/cwn/README.mdpublic_html/webdoors/cwn/index.htmlpublic_html/webdoors/cwn/js/cwn.jspublic_html/webdoors/cwn/webdoor.jsonscripts/migration.phpscripts/setup.phpscripts/upgrade.phpsrc/AdminController.php
✅ Files skipped from review due to trivial changes (7)
- public_html/webdoors/cwn/webdoor.json
- public_html/webdoors/cwn/README.md
- public_html/webdoors/cwn/index.html
- public_html/webdoors/cwn/js/cwn.js
- docs/proposals/CWN_Proposal.md
- docs/DEVELOPER_GUIDE.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/UPGRADING_1.9.5.md
The Add Link form now includes an editable File Name field. It pre-populates from the URL path on blur (using the hostname as fallback when the path has no file extension), and updates to a sanitized page title when Fetch Info is used — fixing vague names like "watch" that appear for YouTube-style URLs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rk in area names Echo area tags like WHAT'S_HOT! contain ' and ! which were excluded from the route regex, causing a 404 before the handler was ever reached. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Echo area tags like WHAT'S_HOT! contain an apostrophe which broke the single-quoted string literals in the Twig-rendered JS block. Switch to json_encode so all special characters are safely escaped. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without |raw, Twig double-encodes the JSON output (e.g. " → "), causing a script parse error and currentEchoarea to be undefined. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two parallel user management pages existed at the same URL (/admin/users): a legacy admin_users.twig (web-routes.php) and a newer admin/users.twig (admin-routes.php). The legacy template was the one actually served and contains features the newer one lacks (pending registrations, reminders, credit grants in the edit modal). - admin-routes.php /users now renders admin_users.twig (the correct one) - Remove /users-new staging route (was pointing at the dead template) - Remove duplicate /admin/users route from web-routes.php - Delete templates/admin/users.twig (dead code) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The player div was being inserted inside the inline span, causing Chrome to auto-promote it past the trailing <br> blank-line separators, leaving a visual gap above the player. Now detects span.message-line and span.message-signature (in addition to <pre>) and inserts the player directly after the containing element, before any blank-line <br>s. Bump sw.js to binkcache-v833. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Promote Admin → LovlyNet as the primary post-registration management interface, add a table of contents, and restructure the doc so the web manager section appears early (after registration) rather than buried inside the AreaFix section. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous instructions implied the setup tool could claim an existing manually-assigned node number, which is not possible without an API key or registry record. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a pen icon beside SoundCloud and Twitter in the media player provider list. Clicking it opens a modal to enter the provider's API key (SoundCloud Client ID, Twitter Bearer Token). Keys are held in memory and included in the payload when Save is clicked. Also adds docs/MediaInMessages.md documenting the full media rendering feature, and wires the missing api_keys field into saveMessageReader(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…shed systems Systems originally seeded from postgres_schema.sql never had early semantic migrations individually recorded in database_migrations. The v5257b1a switch from version_compare to set-based tracking caused those migrations (e.g. 1.4.9) to appear pending again. Restore the implicit-skip rule: legacy semantic migrations whose version <= the highest recorded semantic version are treated as already applied. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…xists The JS check `mp.enabled !== false` treated undefined (missing config) as true. Changed to `!!mp.enabled` so it defaults to false when no config file is present. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lobally Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…expand - Add allow_media column support on echoareas: admins can allow, deny, or inherit the global inline media setting per area; MessageHandler and echomail.js pass area_allow_media through to the media player scan - Media player load button replaced with a context menu (showMediaMenu) giving users direct load, copy-link, and open-in-tab options - Change the user default for media_render_mode from 'auto' to 'click' so new users see click-to-expand rather than auto-rendering by default - BinkpConfig and binkp_config admin template updates - i18n keys added across all locales; docs updated; sw.js cache bumped Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Migration was setting existing rows to 'auto' but the user default is now 'click' (click-to-expand). Update UPGRADING_1.9.5.md to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Uplinks with no allow_media key now deny media instead of allowing it. Checkbox in the admin modal defaults to unchecked for new uplinks. Docs updated to reflect the deny-by-default behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ection - EchoareaManager: fetch allow_media in annotateAreasWithLocalStatus, expose as local_allow_media, add updateAllowMedia() - LovlyNetClient: extend applyRecommendedSettings echo branch to apply allow_media alongside sysop_only - admin-routes: flag allow_media mismatch in annotateLovlyNetAreasWithMetadataIssues - lovlynet.twig: render allow_media issue messages - lovlynet_setup.php: default new LovlyNet echoareas to allow_media=true - i18n: add issue_allow_media_on/off keys to all locales Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add MessageHandler::getEchomailStats() to compute total, unread, read, to-me, saved, and recent counts in a single query using the same visibility rules (moderation, ignore filters, future-date suppression) as the message list. Both stats endpoints now delegate to this method, fixing incorrect "to me" counts that were returned by the old inline route code. - Fix the tome filter in the echomail message list so it delegates to getEchomail() and returns only messages actually addressed to the user. - Fix the admin echomail bulk-delete endpoint to recalculate echoareas.message_count after deletion. Previously the cached counter was never updated, leaving inflated counts in the echoarea list. - Add --fix flag to scripts/check_message_counts.php to repair any message_count drift accumulated before this fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Prevents message_count from being incremented when the echomail INSERT returns no row — matching the existing guard in postEchomail. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Inline Media Player
Message Search
Message Reader
fkey shortcut for toggling full-screen no longer intercepts the browser's Ctrl+F (or Cmd+F on Mac) find shortcut. Ctrl+F and Cmd+F now open the browser's native find dialog as expected. Applies to both echomail and netmail message modals.Content and Display
LOCAL.TESTinstead of appending an empty@suffix.config/i18n/now appears automatically in the telnet and SSH settings language list.index.fr.md), Spanish (index.es.md), Italian (index.it.md), and German (index.de.md). These are served automatically when the user's active language matches the locale.Terminal Registration
File Areas